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

Add reaching definitions analysis #181

Merged
merged 72 commits into from
Apr 29, 2022
Merged

Add reaching definitions analysis #181

merged 72 commits into from
Apr 29, 2022

Conversation

al3xliu
Copy link
Collaborator

@al3xliu al3xliu commented Feb 23, 2022

No description provided.

@al3xliu al3xliu mentioned this pull request Feb 23, 2022
@al3xliu
Copy link
Collaborator Author

al3xliu commented Feb 24, 2022

image

I think I need to change the test case to let the image adjust the pdf...

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

A first round of comments.

Copy link
Collaborator

@zcai1 zcai1 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my previous feedback. Maybe you can also take a look at my comments in #177 because these two PRs have similar structure.

@wmdietl wmdietl changed the title add reach definition in dfa wild vaild contributor Add reaching definitions analysis Mar 1, 2022
Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

File .gitignore needs to add the new generated files as ignorable. After running the tests, git status shouldn't show Out.txt and the class files in the new test directory as new.

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks! Another round of comments.


/**
* A ReachingDefinitionsNode contains a CFG node, which can only be a AssignmentNode. It is used to
* represent the estimate of reaching definitions at certain CFG block during dataflow analysis. We
Copy link
Member

Choose a reason for hiding this comment

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

The plural in reaching definitions is odd, as there is only a single AssignmentNode field. So should this be singular, in the description and class name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, thanks!

ReachingDefinitionsNode generatedDefNode = it.next();
// It's preferred to use "==" to compare two nodes in checker framework,
// but in this case we use `equals` to only measure value equality.
// If we use "==", two expressions from different nodes with same
Copy link
Member

Choose a reason for hiding this comment

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

Can you try to reword? Something reads odd here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

}

/**
* Add the information of a reaching definition into the reaching definitions set.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Add the information of a reaching definition into the reaching definitions set.
* Add a reaching definition to the reaching definitions set.

/** We do not call widenedUpperBound in this analysis. */
@Override
public ReachingDefinitionsStore widenedUpperBound(ReachingDefinitionsStore previous) {
throw new BugInCF("wub of reaching definitions get called!");
Copy link
Member

Choose a reason for hiding this comment

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

Odd wording.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! According to BusyExpr, I removed it.

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks!
In the end run ./gradlew reformat to fix the formatting issues.

Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks!

@wmdietl wmdietl merged commit 66200c2 into eisop:master Apr 29, 2022
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