Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Reverse lookup for DataFlowAnalyzer #14112
base: develop
Are you sure you want to change the base?
Reverse lookup for DataFlowAnalyzer #14112
Changes from all commits
0d1ec65
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Something felt off to me about this file and I finally realized why. It looks too clean. We can't have such nice things here :P You must add the ugly license boilerplate.
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.
There's an easy micro-optimization here - you're looking up
reference
3 times, while you could instead just find an iterator to the element and then use that. Similar with_variable
- 2 separate lookups.It's a fixed number of times so it won't change the complexity of the whole algorithm but it's very easy to do. I'm curious if it will make any kind of difference in benchmark results given that we perform this operation a lot. The lookup is something like
O(log n)
so nothing compared to the linear search we had before but still worth a try.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.
This naming is a bit confusing. What specifically is ordered here? Its type is
unordered_map
so it surely can't be referring to the order of elements, can it?Maybe something like
m_assignments
andm_uses
would be better names? Or maybem_lValues
andm_rValues
?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's ordered in the sense that it's the opposite of reversed.
m_assigments
andm_uses
isn't really correct either, e.g. if we havethen
(a,x)
and(a,y)
could be assignments - but what are the uses?(x, a)
, (y, a)? That doesn't really make sense to me either.
lvaluesand
rvalues` do make sense, but are then somewhat confusing in terms of C++ semantics. In any case, I've spend quite a while trying to come up with names for these, and I'm still convinced these are the best, especially since the whole purpose of this PR is to implement a reverse lookup.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.
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 say that
erase(x)
is exactly the same asset(x, {})
?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.
Eh, yes and no - since we do
m_ordered[_variable] = _references;
, which will still make an insertion for keyx
. I don't think it would ultimately affect the behaviour. I could insert an empty check for_references
however, in which case your statement would be fully correct.edit: Actually, I'm assuming you already knew that this will insert a key with an empty value, so yes, it's exactly the same as
set(x, {})
.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 get your answer here. You're saying that it's not the same but then that it's the same after all. It doesn't look the same to me.
I'd add this check because I don't think we care about distinguishing
x
being assigned an empty set of variables from not being assigned anything. The former can't even be expressed in the language. With this check the behavior of the container will be more consistent - currently withgetOrderedOrNullptr()
you have to check for an empty set explicitly, while withgetReversedOrNullptr()
you can assume you'll always getnullptr
instead.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.
Actually, now that I think about it, it can be expressed after all.
x
can just be assigned a constant expression that does not depend on other variables. So yeah, depends on whether we want the ability to express that. Does not seem to me like we're using that distinction for anything currently.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.
The edit's the final answer - i.e. yes, it's exactly as Chris suggested; adding the empty
_references
check would alter the behaviour of the analysis (I would assume we'd see failing tests, but I'd have to check). I.e. we fetch by key, and then use the value set to either perform arithmetic (i.e. add two sets together), or lookup, neither of which need an empty set check.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.
ok, you're right about this altering the analysis.
But I'm still confused as to why you think
erase(x)
andset(x, {})
would be equivalent. This just does not seem true to me. Are you referring to the fact thatm_ordered[_variable]
will modifym_ordered
and insert the key if it's not there? You're still doingm_ordered.erase()
at the end of the function so yeah, it will technically insert the key but the key won't be there when the function finishes. So I don't think it's true thatThe behaviour is the same as ``set("x", {})``.
. This bit should be removed from the docstring unless I'm missing something here.