-
Notifications
You must be signed in to change notification settings - Fork 161
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
Migrate to bison parser #2185
Migrate to bison parser #2185
Conversation
Needs a changelog entry and I asked @jedelbo to double check the C++ code in case we're missing something about the new API. |
wrappers/src/results_cs.cpp
Outdated
DescriptorOrdering ordering; | ||
query_builder::apply_ordering(ordering, query.get_table(), result.ordering); | ||
return new Results(realm, std::move(query), std::move(ordering)); | ||
return new Results(realm, results.get_query().and_query(std::move(parsed_query)), new_order); |
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.
std::move(new_order)
?
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.
I don't know, to be honest. I noticed that as well but @ironage didn't use it in the example. I thought it was a stylistic error, but then I went to check and only Query
has a custom move constructor, while DescriptorOrdering
has only the default one. After seeing that I thought that it was not a mistake but a wanted choice.
What is your opinion?
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.
I don't know - I'm not a C++ dev 😄 As mentioned, I always add std::move
when I want to move something, just to be on the safe side. Maybe @ironage can educate us on the pros/cons of std::move
-ing the ordering?
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.
That feedback would be very welcome. 🙂
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.
Yes you can move the ordering, I just forgot. Either way is functionally correct, but moving it is an efficient transfer of ownership, while not moving it invokes the copy constructor which inefficiently makes copies of all the contents into the new object only to then discard the previous object entirely.
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.
My only doubt was coming from the fact that while brushing on std::move
I read that returning automatic local variables generally (not sure if always) uses the move semantics instead of the copy ones, as long as a move constructor and assignment exist. So I was not sure if stylistically people still added a std::move
on the returned values. You clearly answered that people generally prefer to be explicit, thank you for the feedback.
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.
Oops, hit approve too fast - still needs a user-facing changelog entry.
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.
Final nit from me. Otherwise, should be ready to merge once realm/realm-core#4290 goes in.
Co-authored-by: Nikola Irinchev <irinchev@me.com>
TODO