-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8224922: Access JavaFileObject from Element(s) #5038
Conversation
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
/csr needed |
@jddarcy this pull request will not be integrated until the CSR request JDK-8272106 for issue JDK-8224922 has been approved. |
Webrevs
|
Seems useful to me. A possible conversion from Element to FileObject is needed elsewhere as well, I've tried to write it here: jdk/src/jdk.compiler/share/classes/com/sun/tools/javac/processing/JavacFiler.java Line 526 in aa61e59
|
@jddarcy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Keep-alive. |
@jddarcy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Still keep-alive. |
Improvements to JDK-8224922
|
…class' FileObject, adding tests for unnamed module/package and automatic modules.
Adding tests for unnamed module/package and automatic modules.
Adding forgotten file/fixing test.
Fixing a test on Windows.
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.
Nice to see at least partial cleanup of the Symbol.classfile
field.
@@ -36,7 +36,7 @@ | |||
* appearing inside method bodies, such as local variables and | |||
* anonymous classes. | |||
* | |||
* <p>When used in the context of annotation processing, an accurate | |||
* <p><a id="accurate_model">When used in the context of annotation processing, an accurate |
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.
It would be more correct to just put the id on the <p>
tag and drop the <a>
.
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.
Fixed.
* (implicitly) created during an annotation processing run from | ||
* the creation of source or class files in that package. An | ||
* {@linkplain PackageElement#isUnnamed unnamed package} will have | ||
* a null file since it cannot be declared in a compilation unit. |
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.
{@code }
around null ??
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'll fix these before pushing and do a follow-up pass over the API for consistent "null" vs "{@code null}" treatment.
* | ||
* <p>If it has a file object, the file object for a module will | ||
* be a {@code module-info} file. An {@linkplain | ||
* ModuleElement#isUnnamed unnamed module} will have a null file |
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.
another null.
* ModuleElement#isUnnamed unnamed module} will have a null file | ||
* since it cannot be declared in a compilation unit. An | ||
* {@linkplain #isAutomaticModule automatic module} will have a | ||
* null file since it is implicitly declared. |
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 guess you're consistent!
@jddarcy This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 4ff4301. |
Initial review to get some comments on the shape of the API. Tests are needed of course, as well as some tuning of the spec to better describe differences in behavior when class files rather than source files are the backing file type.
Progress
Issue
Reviewers
Contributors
<jlahoda@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5038/head:pull/5038
$ git checkout pull/5038
Update a local copy of the PR:
$ git checkout pull/5038
$ git pull https://git.openjdk.java.net/jdk pull/5038/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5038
View PR using the GUI difftool:
$ git pr show -t 5038
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5038.diff