Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1143 +/- ##
==========================================
- Coverage 82.74% 82.18% -0.56%
==========================================
Files 53 53
Lines 7939 7983 +44
Branches 1239 1244 +5
==========================================
- Hits 6569 6561 -8
- Misses 1260 1311 +51
- Partials 110 111 +1 |
CHANGELOG.md
Outdated
|
|
||
| ## Unreleased | ||
|
|
||
| - Add map item iterators. ([#1143](https://github.com/getsentry/sentry-native/pull/1143)) |
There was a problem hiding this comment.
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.
| sentry_value_t | ||
| sentry_value_item_iter_get_value(sentry_item_iter_t *item_iter) | ||
| { | ||
| if (item_iter->index >= *item_iter->len) { |
| if (item_iter->index >= *item_iter->len) { | ||
| return sentry_value_new_null(); | ||
| } | ||
| return item_iter->pairs[item_iter->index].v; |
| int | ||
| sentry_value_item_iter_valid(sentry_item_iter_t *item_iter) | ||
| { | ||
| return item_iter->index < *item_iter->len && item_iter->pairs != NULL; |
There was a problem hiding this comment.
If we check pairs here, we should check len too.
| if (item_iter->frozen || item_iter->index >= *item_iter->len) { | ||
| return 1; | ||
| } | ||
| obj_pair_t *pair = &item_iter->pairs[item_iter->index]; |
There was a problem hiding this comment.
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.
| } | ||
| sentry_free(it); | ||
| } | ||
|
|
There was a problem hiding this comment.
If we keep the mutating iterator, please also add a loop that adds keys or documents that this is forbidden.
Co-authored-by: Mischan Toosarani-Hausberger <mischan@abovevacant.com>
2 participants
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