-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C++: Tidy up SubBasicBlocks.qll #1827
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
Conversation
These files are now added to `identical-files.json` so they will remain in sync.
Previously, the word "position" was used ambiguously in this library.
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.
Changes all LGTM. Thanks especially for using "rank"/"index" instead of "pos".
This predicte was still used in a test, so it might be used in external code too.
Added two commits to fix the failing test. |
* returns a 0-based position, while `getRankInBasicBlock` returns a 1-based | ||
* position. | ||
*/ | ||
deprecated int getPosInBasicBlock(BasicBlock bb) { |
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.
Why are we adding a deprecated predicate? Is there another part to this change?
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.
... ah, looks like I was viewing an old version of the PR, or possibly a single commit.
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.
LGTM.
Time to merge this I think. |
This series of commits tidies up my rather crude performance fixes from #1824. I decided to leave one
pragma[nomagic]
on a predicate that seemed to be sensitive to context in my experiments. TheSubBasicBlocks
library is intended to be used in many different contexts.This PR does not have the 1.22 milestone. I believe the library in 1.22 is performant, correct, and well tested.