feat: Map item iterators #1143

limbonaut · 2025-02-06T13:43:55Z

This PR introduces map item iterators. Iterator can be used to traverse the map (aka object) key-value pairs. Iterators can also be used to selectively erase items (aka filtering).

Rationale

There is currently no way to get the keys of a map object. We need such a function to build proper representations in upstream SDKs (like sentry-godot). See #1142 for more details.

Supercedes #1142

github-actions · 2025-02-06T13:44:16Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against dcc5f20

codecov · 2025-02-06T14:11:33Z

Codecov Report

Attention: Patch coverage is 0% with 44 lines in your changes missing coverage. Please review.

Project coverage is 82.13%. Comparing base (03143a8) to head (dcc5f20).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1143      +/-   ##
==========================================
- Coverage   82.66%   82.13%   -0.54%     
==========================================
  Files          53       53              
  Lines        7927     7971      +44     
  Branches     1239     1244       +5     
==========================================
- Hits         6553     6547       -6     
- Misses       1263     1312      +49     
- Partials      111      112       +1

supervacuus · 2025-02-11T14:33:21Z

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog

+## Unreleased
+
+- Add map item iterators. ([#1143](https://github.com/getsentry/sentry-native/pull/1143))


I know that the comments in the header use "map" quite often to talk about interfaces related to the "object" value type, but I would prefer to call this an "object item iterator", given that "object" is the nomenclature that the API uses, especially in the change log.

supervacuus · 2025-02-11T14:33:57Z

include/sentry.h

@@ -244,6 +244,50 @@ SENTRY_API int sentry_value_remove_by_key(sentry_value_t value, const char *k);
 SENTRY_API int sentry_value_remove_by_key_n(
    sentry_value_t value, const char *k, size_t k_len);

+/**
+ * Map item iterator.


Suggested change

* Map item iterator.

* Object item iterator.

supervacuus · 2025-02-11T14:34:15Z

include/sentry.h

+/**
+ * Map item iterator.
+ *
+ * It's used to iterate over the key-value pairs of a map.


Suggested change

* It's used to iterate over the key-value pairs of a map.

* It's used to iterate over the key-value pairs of an object.

supervacuus · 2025-02-11T14:34:32Z

include/sentry.h

+typedef struct sentry_item_iter_s sentry_item_iter_t;
+
+/**
+ * Creates a new map item iterator.


Suggested change

* Creates a new map item iterator.

* Creates a new object item iterator.

supervacuus · 2025-02-11T14:34:57Z

include/sentry.h

+/**
+ * Creates a new map item iterator.
+ *
+ * Returns NULL if the given value is not a map.


Suggested change

* Returns NULL if the given value is not a map.

* Returns `NULL` if the given value is not an object.

supervacuus · 2025-02-11T14:50:34Z

src/sentry_value.c

+sentry_value_t
+sentry_value_item_iter_get_value(sentry_item_iter_t *item_iter)
+{
+    if (item_iter->index >= *item_iter->len) {


supervacuus · 2025-02-11T14:50:42Z

src/sentry_value.c

+    if (item_iter->index >= *item_iter->len) {
+        return sentry_value_new_null();
+    }
+    return item_iter->pairs[item_iter->index].v;


supervacuus · 2025-02-11T14:51:53Z

src/sentry_value.c

+int
+sentry_value_item_iter_valid(sentry_item_iter_t *item_iter)
+{
+    return item_iter->index < *item_iter->len && item_iter->pairs != NULL;


If we check pairs here, we should check len too.

supervacuus · 2025-02-11T15:05:16Z

src/sentry_value.c

+    if (item_iter->frozen || item_iter->index >= *item_iter->len) {
+        return 1;
+    }
+    obj_pair_t *pair = &item_iter->pairs[item_iter->index];


Again, do we assume that pairs and len are valid pointers in this case?

If so, why? Because valid() does the check? If so, we must decide whether users must check for validity every time either the iterator or the object changed and erase(), get_key() and get_value() assume valid iterators or add the checks in all of those functions.

Whichever way we choose, we should be consistent regarding the guarantees of calling valid(), the accessors, and mutators (since adding new keys will then also be used if iterators allow object mutation).

This demonstrates how easily checks (and thus guarantees) can become inconsistent quickly when an in-place erasure is introduced. If you introduce the guarantees for each getter and mutator (making valid() solely a function for a loop-invariant), then you can't change that without breaking code. OTOH, adding those guarantees later won't break any code.

supervacuus · 2025-02-11T15:45:57Z

tests/unit/test_value.c

+        }
+        sentry_free(it);
+    }
+


If we keep the mutating iterator, please also add a loop that adds keys or documents that this is forbidden.

supervacuus and others added 2 commits February 6, 2025 12:05

Base implementation

cd8fbd6

Add sentry_value_item_iter_remove_item(it)

bf04599

limbonaut mentioned this pull request Feb 6, 2025

feat: Add sentry_value_get_key(value, index) #1142

Closed

limbonaut added 11 commits February 7, 2025 11:47

Handle frozen objects

2b7c381

Add descriptions

d1f42e4

Test iteration

0572237

Test item erase

338913c

make format

1ee402d

Fix tests

eb2bbb1

Fix and expand tests

5254d25

Formatting

c0974e3

Fix blunder

7512069

Update comments

1a1cb69

Update CHANGELOG

dcc5f20

limbonaut mentioned this pull request Feb 7, 2025

feat: SentryBreadcrumb class and before_breadcrumb hook getsentry/sentry-godot#83

Draft

3 tasks

limbonaut marked this pull request as ready for review February 7, 2025 14:45

limbonaut requested a review from supervacuus February 7, 2025 14:46

supervacuus reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Map item iterators #1143

feat: Map item iterators #1143

limbonaut commented Feb 6, 2025 •

edited

Loading

github-actions bot commented Feb 6, 2025 •

edited

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

supervacuus Feb 11, 2025

	* It's used to iterate over the key-value pairs of a map.
	* It's used to iterate over the key-value pairs of an object.

	* Creates a new map item iterator.
	* Creates a new object item iterator.

	* Returns NULL if the given value is not a map.
	* Returns `NULL` if the given value is not an object.

feat: Map item iterators #1143

Are you sure you want to change the base?

feat: Map item iterators #1143

Conversation

limbonaut commented Feb 6, 2025 • edited Loading

Rationale

github-actions bot commented Feb 6, 2025 • edited Loading

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limbonaut commented Feb 6, 2025 •

edited

Loading

github-actions bot commented Feb 6, 2025 •

edited

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading