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 live variable analysis to dataflow framework. #3371

Merged

Conversation

xingweitian
Copy link
Member

@xingweitian xingweitian commented Jun 13, 2020

This PR is based on and should be reviewed after #3370. This PR added live variable analysis to the dataflow framework aiming to show #3370's backward analysis works properly.

Co-authored-by: Charles Chen Charleszhuochen@gmail.com

xingweitian and others added 2 commits June 12, 2020 23:44
Co-authored-by: Charles Chen <Charleszhuochen@gmail.com>
Co-authored-by: Charles Chen <Charleszhuochen@gmail.com>
@xingweitian xingweitian assigned kelloggm and unassigned xingweitian Jun 26, 2020
smillst pushed a commit that referenced this pull request Jun 30, 2020
Refactor dataflow framework. Change Analysis to be an interface, and add AbstractAnalysis,
ForwardAnalysis, ForwardTransferFunction, ForwardAnalysisImpl, BackwardAnalysis, BackwardTransferFunction, and BackwardAnalysisImpl.

Live variable analysis is also added in #3371 to show that the introduced backward analysis can work.

Co-authored-by: Charles Chen Charleszhuochen@gmail.com
@smillst
Copy link
Member

smillst commented Jun 30, 2020

#3370 has been merged to master. @xingweitian Can you merge typetools/master into this branch so the diff is smaller?

@smillst smillst assigned xingweitian and unassigned smillst Jun 30, 2020
@xingweitian xingweitian assigned smillst and unassigned xingweitian Jun 30, 2020
/**
* Classes using for live variable analysis. Live variable analysis is a backward analysis to
* calculate the variables that are live at each point in the program. To run live variable
* analysis, see {@link org.checkerframework.dataflow.cfg.playground.LiveVariablePlayground}.
Copy link
Member

Choose a reason for hiding this comment

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

I think this should just show the code in LiveVariablePlayground. This is a link to the Javadoc which doesn't explain how to use the analysis. The just delete LiveVariablePlayground as it contents are identical to LiveVariable in the test directory.

More generally, do you expect people to use this analysis or this just for testing? If it's for testing, it should be moved to src/test/java. If it's for general use, should it be mentioned in the dataflow manual?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment! I would expect people to run the LiveVariablePlayground. It now has been mentioned in the manual.
I am not sure what to do with this package-info.java, I just wanted to point the users to LiveVariablePlayground. Do you mean that I need to copy and paste LiveVariablePlayground's code here?

Copy link
Member

Choose a reason for hiding this comment

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

I see the livevariable backwards analysis like the constantpropagation forward analyses: demo analyses for the dataflow framework that are only used in their respective playgrounds. Either one could be useful to users.
If we decide to move the livevariable implementation we should also move the constantpropagation and the playgrounds to a test directory.

I like the suggestion of adding a section to the dataflow manual, like for constantpropagation: https://github.com/typetools/checker-framework/blob/master/dataflow/manual/content.tex#L1699
As this PR contains the tests for the backwards analysis implementation, it might be good to merge it and work on the manual in a separate PR. But it's also not critical to get this PR into the release, so we can work on the manual section in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this PR already adds it to the dataflow manual. So that's not a blocker.

@@ -31,3 +31,31 @@ task deployArtifactsToSonatype {
mvnSignAndDeployMultipleToSonatype(shadowJar.archiveFile.get().toString(), sourcesJar.archiveFile.get().toString(), javadocJar.archiveFile.get().toString(), "${pomFiles}/dataflow-shaded-pom.xml")
}
}

task liveVariableTest(dependsOn: compileTestJava, group: 'Verification') {
Copy link
Member

Choose a reason for hiding this comment

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

FYI, I rewrote the Makefile test to a Gradle test.

@smillst smillst assigned xingweitian and unassigned smillst Jun 30, 2020
@xingweitian xingweitian assigned smillst and unassigned xingweitian Jul 1, 2020
Copy link
Member

@smillst smillst left a comment

Choose a reason for hiding this comment

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

Thanks! I'll merge this once it's passing so it's in todays release.

@smillst smillst merged commit 7558863 into typetools:master Jul 1, 2020
@xingweitian xingweitian deleted the typetools-live-variable-analysis branch July 8, 2020 20:59
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