Skip to content

Python ESSA: Fix performance of PythonSsaSourceVariable.hasRefinementEdge()#600

Merged
taus-semmle merged 1 commit into
github:rc/1.19from
markshannon:python-fix-performance-refinement-edge
Dec 4, 2018
Merged

Python ESSA: Fix performance of PythonSsaSourceVariable.hasRefinementEdge()#600
taus-semmle merged 1 commit into
github:rc/1.19from
markshannon:python-fix-performance-refinement-edge

Conversation

@markshannon

Copy link
Copy Markdown
Contributor

No description provided.

@taus-semmle taus-semmle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two small questions, but otherwise LGTM.

}

override predicate hasRefinementEdge(ControlFlowNode use, BasicBlock pred, BasicBlock succ) {
test_contains(pred.getLastNode(), use) and

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Compared to what the code did previously, test_contains contains an additional instanceof Expr check. It's not immediately clear to me whether this affects the behaviour. Can you weigh in on this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The last node in a branching block will always be an Expr. So this doesn't change behaviour, just makes it faster.

n = this.getAUse() or
this.hasDefiningNode(n) |
pred.(ConditionBlock).strictlyReaches(n.getBasicBlock())
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we know which of these changes -- using test_contains and removing this conjunct -- was responsible for the improvement? Was it both?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both, but mainly this one, I think.

@taus-semmle taus-semmle merged commit 08b1431 into github:rc/1.19 Dec 4, 2018
cklin pushed a commit that referenced this pull request May 23, 2022
Improve "Incorrect integer conversion" query
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants