-
Notifications
You must be signed in to change notification settings - Fork 352
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
Add live variable analysis to dataflow framework. #3371
Conversation
Co-authored-by: Charles Chen <Charleszhuochen@gmail.com>
Co-authored-by: Charles Chen <Charleszhuochen@gmail.com>
Reduce the differences between forward and backward analysis implementation.
…o typetools-live-variable-analysis
Tweaks the javadoc.
…o typetools-live-variable-analysis
…o typetools-live-variable-analysis
…o typetools-live-variable-analysis
…o typetools-live-variable-analysis
This reverts commit ef29c77
…' into typetools-live-variable-analysis
Update Expected.txt.
Use block instead of bb as parameter name.
…o typetools-backward-analysis
…o typetools-live-variable-analysis
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
#3370 has been merged to master. @xingweitian Can you merge typetools/master into this branch so the diff is smaller? |
dataflow/src/main/java/org/checkerframework/dataflow/cfg/StringCFGVisualizer.java
Outdated
Show resolved
Hide resolved
/** | ||
* 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}. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
dataflow/src/main/java/org/checkerframework/dataflow/livevariable/LiveVar.java
Outdated
Show resolved
Hide resolved
dataflow/src/main/java/org/checkerframework/dataflow/livevariable/LiveVar.java
Outdated
Show resolved
Hide resolved
@@ -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') { |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
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