Expose `list_find` in the c api #5848

nicola-cab · 2022-09-13T11:37:30Z

What, How & Why?

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

nicola-cab · 2022-09-13T11:40:18Z

test/object-store/c_api/c_api.cpp

@@ -2624,6 +2664,13 @@ TEST_CASE("C API", "[c_api]") {
                    size_t size;
                    CHECK(checked(realm_list_size(bars.get(), &size)));
                    CHECK(size == 2);
+


@ironage @jedelbo I noticed for all the CollectionBase objects we don't allow searching with, type type_TypedLink . Is this still wanted? Or it is something we should enable?

A list of TypedLink is not officially supported. But there should be some way to find a destination object in a list of links. In C++ we support finding by ObjKey, but it looks like the C-API doesn't expose this type in a realm_value_t for some reason. We should either convert TypedLink to ObjKey in realm_list_find or we should support LnkLst::find_any given a Mixed of type TypedLink. In either case, when converting from TypedLink to ObjKey, we'd need to check that the destination table matches.

I think probably we should file a separate issue and add support for this stuff at some stage in C API too.

I just added a small fix, I think it is OK to sneak in here.

OK, then we need to do this also for Sets and Dictionaries, right?

I think we need to do nothing for Dictionaries. I added the implementation for Sets

nirinchev · 2022-09-13T18:28:23Z

src/realm.h

+/**
+ * Find the value in the list passed as parameter.
+ * @param value to search in the list
+ * @param out_index the index where the value has been found


What is this set to if the value hasn't been found?

realm::not_found;

I think we should document the actual value here to make it easier for consumers or alternatively, define it as a constant somewhere so that it can be referenced.

I can add a comment, but this is in line with all the other find APIs

…m_list_find

jedelbo · 2022-09-14T12:43:27Z

src/realm/set.hpp

-        found = real2virtual(found);
+
+    auto find_obj_key = [this](ObjKey objkey) {
+        size_t found = find(objkey);


This find method makes the translation to virtual index. Just use it directly below. This seems to have been an error before as well. Do we actually have a test involving this? I played around to verify my assumptions - may as well push my findings.

nirinchev · 2022-09-14T12:53:58Z

src/realm.h

+ * @param out_found boolean that indicates whether the value is found or not
+ * @return true if no exception occurred.
+ */
+RLM_API bool realm_list_find(const realm_list_t*, const realm_value_t* value, size_t* out_index, bool* out_found);


Do we need out_found - seems like we can easily infer that from out_index.

Yes we do and I disagree on this proposal. For 2 reasons:

finds for set has the same signature (and also most of the find APIs we have in the C API)

out_index is of type size_t this means it can never be negative. If you assign -1 to signal that the value was not found, this index might be MAX_LIMIT(std::size_t) which can be a valid index.
I would leave it as it is.

Actually, realm::not_found is std::size{-1}. So in theory, we could get rid of the boolean. However, I would rather keep it as it is to keep it consistent.

nirinchev · 2022-09-14T12:56:28Z

src/realm.h

+/**
+ * Find the value in the list passed as parameter.
+ * @param value to search in the list
+ * @param out_index the index where the value has been found


I think we should document the actual value here to make it easier for consumers or alternatively, define it as a constant somewhere so that it can be referenced.

nirinchev · 2022-09-14T12:57:50Z

test/object-store/c_api/c_api.cpp

+
+                        size_t out_index = -1;
+                        bool found;
+                        CHECK(checked(realm_list_find(strings.get(), &a2, &out_index, &found)));


These tests only seem to test the case where a value is found. Can we add a test where find is called with a value that doesn't exist in the list?

This was actually a test that was exercising the "not found" path, but we are sneaking in Mixed Links support as well. Let me add a test.

jedelbo

LGTM

nirinchev · 2022-09-14T16:19:26Z

I noticed we're also missing results_find - not sure if you prefer to add it in this PR or we could do a separate issue for it.

nicola-cab · 2022-09-14T17:13:57Z

I noticed we're also missing results_find - not sure if you prefer to add it in this PR or we could do a separate issue for it.

There are 2 versions of realm_results_get.
realm_results_get and realm_results_get_object one is for links and the other one is for objects, which is a little bit odd, but the 2 APIs, although they do the same thing, they have different signatures. Probably we should just reform these APIs a little bit. Do we want to add two types of realm_results_find? Maybe this is worth another issue.

nirinchev · 2022-09-14T18:53:39Z

Good point - it's weird we have two of those. I'll file a new ticket to address that and add a single results_find method.

…nification * origin/master: Stop forcing enums to be 64 bits unnecessarily clean up documentation of internal fields in config structs SyncConfig should be default constructible Traversal functions use IteratorControl values rather than true/false which is more expressive (#5857) Fix handling of 4-byte UTF8 values on Windows (#5803) Encode links in a way the server can understand (#5835) expose `Group::remove_table` in the C API (#5860) Disable auto refresh for old realm during migration (#5856) Expose `list_find` in the c api (#5848) Do not allow asymmetric tables in pbs (#5853) Refactor link tracing code (#5796) Expose `Obj::get_parent_object` in the C API (#5851) Update app.hpp (#5854) Fix appending to list ignores existing query (#5850)

expose list_find in the c api

4ff19f3

nicola-cab requested review from jedelbo and ironage September 13, 2022 11:37

cla-bot bot added the cla: yes label Sep 13, 2022

changelog entry

a17f6e1

nicola-cab commented Sep 13, 2022

View reviewed changes

fix compilation warning

0845c4e

nirinchev reviewed Sep 13, 2022

View reviewed changes

jedelbo and others added 3 commits September 14, 2022 11:52

Support finding a typed link in LnkLst

eb49701

Merge branch 'master' of github.com:realm/realm-core into nc/add_real…

1fd91c3

…m_list_find

allow type_TypedLink for sets

1832595

jedelbo reviewed Sep 14, 2022

View reviewed changes

Fix

70f2daf

nirinchev reviewed Sep 14, 2022

View reviewed changes

jedelbo reviewed Sep 14, 2022

View reviewed changes

nicola-cab added 3 commits September 14, 2022 14:59

fix changelog conflicts

7cf317d

added some more testing for element not found

ade26dd

more testing

2783516

ironage approved these changes Sep 14, 2022

View reviewed changes

nirinchev mentioned this pull request Sep 14, 2022

[C-API] Expose results_find and consolidate results_get and results_get_object #5858

Closed

nicola-cab merged commit 4d21f9a into master Sep 14, 2022

nicola-cab deleted the nc/add_realm_list_find branch September 14, 2022 19:51

nielsenko mentioned this pull request Sep 15, 2022

Support list indexOf realm/realm-dart#910

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `list_find` in the c api #5848

Expose `list_find` in the c api #5848

nicola-cab commented Sep 13, 2022

nicola-cab Sep 13, 2022

ironage Sep 13, 2022

nicola-cab Sep 14, 2022

jedelbo Sep 14, 2022

nicola-cab Sep 14, 2022

nicola-cab Sep 14, 2022

nirinchev Sep 13, 2022

nicola-cab Sep 14, 2022

nirinchev Sep 14, 2022

nicola-cab Sep 14, 2022

jedelbo Sep 14, 2022

nirinchev Sep 14, 2022

nicola-cab Sep 14, 2022

nicola-cab Sep 14, 2022

nirinchev Sep 14, 2022

nirinchev Sep 14, 2022

nicola-cab Sep 14, 2022

jedelbo left a comment

nirinchev commented Sep 14, 2022

nicola-cab commented Sep 14, 2022 •

edited

Loading

nirinchev commented Sep 14, 2022

Expose list_find in the c api #5848

Expose list_find in the c api #5848

Conversation

nicola-cab commented Sep 13, 2022

What, How & Why?

☑️ ToDos

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

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

jedelbo left a comment

Choose a reason for hiding this comment

nirinchev commented Sep 14, 2022

nicola-cab commented Sep 14, 2022 • edited Loading

nirinchev commented Sep 14, 2022

Expose `list_find` in the c api #5848

Expose `list_find` in the c api #5848

nicola-cab commented Sep 14, 2022 •

edited

Loading