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

Upgrading the stubparser #1441

Merged
merged 237 commits into from Aug 13, 2017
Merged

Upgrading the stubparser #1441

merged 237 commits into from Aug 13, 2017

Conversation

Bohdankm22
Copy link
Contributor

@Bohdankm22 Bohdankm22 commented Aug 9, 2017

Upgrading the stubparser in the Checker Framework to use the newest JavaParser project. The stubparser will be removed and instead of this javapasrer-core.jar lib will be added in framework/lib

Bohdankm22 and others added 30 commits May 30, 2017 12:43
…vaparser-core.jar;

Resolved all the compilation problems;
Added a bunch of TODOs that should be resolved later regarding the switching the dependency.
…vaparser-core.jar;

Resolved all the compilation problems;
Added a bunch of TODOs that should be resolved later regarding the switching the dependency.
- Added dependency of the framework module on javaparser-core.jar in build.xml
@mernst
Copy link
Member

mernst commented Aug 10, 2017

Looks good; I approve the pull request.
Don't squash-and-merge until tests pass and the CF developers have approved requiring Java 8 for the next release.

@mernst
Copy link
Member

mernst commented Aug 10, 2017

Note: when we squash-and-merge this pull request, include the text:
Fixes #220. Fixes #297. Fixes #375. Fixes #407. Fixes #571. Fixes #1371.

- Removed redundant information in the manual#stub-file-format regarding the imports of enum contants.
@Bohdankm22
Copy link
Contributor Author

I fixed it in the next commit d7c6191

@Bohdankm22
Copy link
Contributor Author

No differences in Java limitations and stub parser requirements regarding the enums declarations.
Hence the entire "Enum declarations:" item should be removed.
I will take care of it.

@mernst mernst merged commit 8997a34 into typetools:master Aug 13, 2017
@mernst mernst deleted the stubtests branch August 13, 2017 11:58
@@ -35,7 +35,7 @@ annoToolsDir="${cfDir}"/../annotation-tools
# Put afu jar files last, as they might contain out-of-date CF files.
# Put "checker" after the other sub-projects, as "ant bindist" puts
# other projects into the checker/build directory.
buildDirs="${cfDir}"/dataflow/build:"${cfDir}"/javacutil/build:"${cfDir}"/stubparser/build:"${cfDir}"/framework/build:"${cfDir}"/checker/build:"${annoToolsDir}"/scene-lib/bin:"${annoToolsDir}"/annotation-file-utilities/annotation-file-utilities.jar
buildDirs="${cfDir}"/dataflow/build:"${cfDir}"/javacutil/build:"${cfDir}"/framework/build:"${cfDir}"/checker/build:"${cfDir}"/checker/lib/javaparser-core.jar:"${annoToolsDir}"/scene-lib/bin:"${annoToolsDir}"/annotation-file-utilities/annotation-file-utilities.jar
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the "${cfDir}"/checker/lib/javaparser-core.jar forgotten here from some older version of this change?
I just got a little bit confused by this addition. Not really a problem, but perhaps better clean it up.

Copy link
Member

Choose a reason for hiding this comment

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

@panacekcz , the pull request removed the stub parser source code from the Checker Framework repository and added, in its place, a dependency on a .jar file built from an external repository. So, adding that dependence is correct.

@feliperodri , should we rename the javaparser-core.jar file to stubparser-core.jar, to emphasize that this is not the real JavaParser but our modified version of it? (However, all the package names are not changed, so it isn't currently possible to use real JavaParser at the same time as the Stub Parser.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mernst , I would definitely suggest you rename the javaparser-core.jar file to stubparser-core.jar. Futures developers might find this confusing, so it is better to make clear it's a different version (even though it isn't possible to use both at the same time right now.)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, that question should have been aimed at @Bohdankm22 . My mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mernst In that case, I think the path should be changed to "${cfDir}"/framework/lib/javaparser-core.jar and added to javac-debug as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@panacekcz, @mernst it's true, the path should be changed because the lib is in the framework/lib/. I will change it and rename to the stubparser-core.jar.

Copy link
Member

Choose a reason for hiding this comment

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

And keep javac in sync with javac-debug, as Vlastimil pointed out. (Thanks; I missed that in my review.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mernst, sure.
Doing the changes in #1451

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.

None yet

5 participants