-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
[WIP] First small optimisation for z-order curve (a<x and x<b => a<x<b) #7327
Conversation
if ( | ||
lhs.function == RPNElement::FUNCTION_IN_RANGE && |
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.
Let's remove this line break.
Can we add some kind of test for this code? |
{ | ||
const size_t to_modify = rpn.size() - 3; | ||
const auto rhs = rpn[to_modify + 1]; | ||
const auto lhs = rpn[to_modify]; |
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.
We can use references here.
Also, let's create a merged element on stack and then put it in rpn
if the merge succeeds -- when modifying in place, we can leave some rubbish, especially if some field is added in the future.
lhs.monotonic_functions_chain == rhs.monotonic_functions_chain) | ||
{ | ||
bool merged = false; | ||
if (!lhs.range.right_bounded && !rhs.range.left_bounded) |
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.
Can we just always use Range::intersectRange
, or is there some case when this doesn't work?
Let's continue? |
Yep, I would soon |
I couldn't reproduce improvement from this optimization and stucked |
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
For changelog. Remove if this is non-significant change.
First small improvement of KeyCondition.
Category (leave one):