-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose more dictionary APIs in the C API #6235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved modulo minor touchups 👍
* @param out_keys the list of keys in the dictionary | ||
* @return True if no exception occurred. | ||
*/ | ||
RLM_API bool realm_dictionary_get_keys(realm_dictionary_t*, size_t* out_size, realm_results_t** out_keys); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is conventional in the C API, but returning ownership of newly allocated memory to the caller seems dangerous and IMO at least warrants a note about this in the doc string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is dangerous, I agree. But it is how the C-API works. The memory has to be freed once it is not used any more. The issue is always the same, we don't have a way to tell the caller if there was a problem due to some exception or if the dictionary was simply empty. We can't use nullptr
for both.
I will add a comment in the doc string.
Co-authored-by: James Stone <james.stone@mongodb.com>
…realm-core into nc/add_support_dictionaries_c_api
Merging this PR since failures are unrelated to the change. There is something odd with the Sync Tests, and most of the tasks timed out. |
What, How & Why?
Fixes: #6181
☑️ ToDos