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

Dominator deadcode problem fix #1984

Merged
merged 12 commits into from
Oct 12, 2023

Conversation

Tiko7454
Copy link
Contributor

Fixes #1973 the way I mentioned in the bug report

@Tiko7454 Tiko7454 force-pushed the dominator-deadcode-problem-fix branch 2 times, most recently from ab43894 to d75d8d8 Compare June 23, 2023 06:10
@CLAassistant
Copy link

CLAassistant commented Jun 23, 2023

CLA assistant check
All committers have signed the CLA.

@Tiko7454 Tiko7454 force-pushed the dominator-deadcode-problem-fix branch from 3e2ed00 to d75d8d8 Compare June 26, 2023 05:14
@Tiko7454 Tiko7454 force-pushed the dominator-deadcode-problem-fix branch from 2de8ab5 to cee0c6b Compare June 26, 2023 05:52
@Tiko7454 Tiko7454 force-pushed the dominator-deadcode-problem-fix branch from 8ae3ada to 7a35d8f Compare June 26, 2023 06:07
@Tiko7454 Tiko7454 closed this Aug 21, 2023
@Tiko7454 Tiko7454 force-pushed the dominator-deadcode-problem-fix branch from 2687867 to 0de7a25 Compare August 21, 2023 12:36
@Tiko7454 Tiko7454 reopened this Aug 21, 2023
@Tiko7454 Tiko7454 force-pushed the dominator-deadcode-problem-fix branch from a15e1fc to 2687867 Compare September 11, 2023 09:07

ret = set()
for pred in node.fathers:
ret = ret.union(pred.dominators)
Copy link
Member

Choose a reason for hiding this comment

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

Mhh I am confused, can you explain why are we doing the union over the predecessor dominators, and then their interaction?

It seem to me that this loop should be removed - unless I am missing something here

Copy link
Contributor Author

@Tiko7454 Tiko7454 Sep 15, 2023

Choose a reason for hiding this comment

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

It's the safest way to start set intersections. I could store the first set as the base and start intersecting all sets with that but the first set may not exist. So the safest way is to get every element from every set, and start to intersect every set with that big set

Copy link
Member

Choose a reason for hiding this comment

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

gotcha that makes sense :)

@montyly montyly merged commit 7242a32 into crytic:dev Oct 12, 2023
73 checks passed
@montyly
Copy link
Member

montyly commented Oct 12, 2023

thanks @Tiko7454

@montyly
Copy link
Member

montyly commented Oct 13, 2023

For visibility, there was an issue with the recent add of the vyper support (see #2171). We will investigate this further

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.

4 participants