Skip to content
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

C++: Tidy up SubBasicBlocks.qll #1827

Merged
merged 7 commits into from
Aug 29, 2019
Merged

C++: Tidy up SubBasicBlocks.qll #1827

merged 7 commits into from
Aug 29, 2019

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Aug 26, 2019

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. The SubBasicBlocks 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.

jbj and others added 5 commits August 26, 2019 16:01
@jbj jbj added the C++ label Aug 26, 2019
@jbj jbj requested review from pavgust and geoffw0 August 26, 2019 19:54
@jbj jbj requested a review from a team as a code owner August 26, 2019 19:54
Copy link
Contributor

@dave-bartolomeo dave-bartolomeo left a 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.
@jbj
Copy link
Contributor Author

jbj commented Aug 28, 2019

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) {
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@geoffw0
Copy link
Contributor

geoffw0 commented Aug 29, 2019

Time to merge this I think.

@geoffw0 geoffw0 merged commit 1215da2 into github:master Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants