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.

Sign up for GitHub

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

Jump to bottom

8266631: StandardJavaFileManager: getJavaFileObjects() impl violates the spec #4360

Closed
wants to merge 2 commits into from
Closed

8266631: StandardJavaFileManager: getJavaFileObjects() impl violates the spec #4360

wants to merge 2 commits into from

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Jun 4, 2021 •
edited by openjdk bot
Loading

This improves javadoc/specification of several StandardJavaFileManager methods, as requested in these bugs:
https://bugs.openjdk.java.net/browse/JDK-8266631
https://bugs.openjdk.java.net/browse/JDK-8266596
https://bugs.openjdk.java.net/browse/JDK-8266591
https://bugs.openjdk.java.net/browse/JDK-8266590

The CSR is here:
https://bugs.openjdk.java.net/browse/JDK-8268260

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issues

  • JDK-8266631: StandardJavaFileManager: getJavaFileObjects() impl violates the spec
  • JDK-8266596: StandardJavaFileManager: default impls of setLocationFromPaths(), getJavaFileObjectsFromPaths() methods don't throw IllegalArgumentException as specified
  • JDK-8266591: StandardJavaFileManager::getJavaFileObjectsFromPaths() methods contain a typo in their spec
  • JDK-8266590: StandardJavaFileManager::setLocationFromPaths() spec contains an error

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4360/head:pull/4360
$ git checkout pull/4360

Update a local copy of the PR:
$ git checkout pull/4360
$ git pull https://git.openjdk.java.net/jdk pull/4360/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4360

View PR using the GUI difftool:
$ git pr show -t 4360

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4360.diff

@lahodaj
Copy link
Contributor Author

lahodaj commented Jun 4, 2021

/issue add JDK-8266631,JDK-8266596,JDK-8266591,JDK-8266590

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 4, 2021

👋 Welcome back jlahoda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 4, 2021
@openjdk
Copy link

openjdk bot commented Jun 4, 2021

@lahodaj This issue is referenced in the PR title - it will now be updated.

Adding additional issue to issue list: 8266596: StandardJavaFileManager: default impls of setLocationFromPaths(), getJavaFileObjectsFromPaths() methods don't throw IllegalArgumentException as specified.

Adding additional issue to issue list: 8266591: StandardJavaFileManager::getJavaFileObjectsFromPaths() methods contain a typo in their spec.

Adding additional issue to issue list: 8266590: StandardJavaFileManager::setLocationFromPaths() spec contains an error.

@openjdk
Copy link

openjdk bot commented Jun 4, 2021

@lahodaj The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the compiler compiler-dev@openjdk.org label Jun 4, 2021
@mlbridge
Copy link

mlbridge bot commented Jun 4, 2021 •
edited
Loading

Webrevs

vicente-romero-oracle
vicente-romero-oracle approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment •
edited
Loading

Choose a reason for hiding this comment

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

the patch and the CSR look good to me. I would just inline the spec changes in the specification section of the CSR

@openjdk
Copy link

openjdk bot commented Jun 4, 2021 •
edited
Loading

@lahodaj This change is no longer ready for integration - check the PR body for details.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 4, 2021
@jonathan-gibbons
Copy link
Contributor

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Jun 4, 2021
@openjdk
Copy link

openjdk bot commented Jun 4, 2021

@jonathan-gibbons this pull request will not be integrated until the CSR request JDK-8268260 for issue JDK-8266631 has been approved.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jun 4, 2021
jonathan-gibbons
jonathan-gibbons suggested changes Jun 4, 2021
View reviewed changes
src/java.compiler/share/classes/javax/tools/StandardJavaFileManager.java Outdated
* The default implementation converts each path to a file and calls
* {@link #getJavaFileObjectsFromFiles getJavaObjectsFromFiles}.
* The default implementation lazily converts each path to a file and calls
* {@link #getJavaFileObjectsFromPaths(Collection) getJavaFileObjectsFromPaths}.
* IllegalArgumentException will be thrown if any of the paths
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're editing lines in this region ... use {@code} or {@link} around IllegalArgumentException.

src/java.compiler/share/classes/javax/tools/StandardJavaFileManager.java Outdated
* The default implementation converts each path to a file and calls
* {@link #getJavaFileObjectsFromFiles getJavaObjectsFromFiles}.
* The default implementation lazily converts each path to a file and calls
* {@link #getJavaFileObjectsFromFiles(Iterable) getJavaFileObjectsFromFiles}.
* IllegalArgumentException will be thrown if any of the paths
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're editing lines in this region ... use {@code} or {@link} around IllegalArgumentException.

src/java.compiler/share/classes/javax/tools/StandardJavaFileManager.java
* IllegalArgumentException will be thrown if any of the paths
* cannot be converted to a file.
* cannot be converted to a file at the point the conversion happens.
*
* @param paths a list of paths
* @return a list of file objects
Copy link
Contributor

Choose a reason for hiding this comment

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

In the following @throws, remove the trailing "."

src/java.compiler/share/classes/javax/tools/StandardJavaFileManager.java Outdated
@@ -267,7 +267,8 @@
* @throws IllegalArgumentException if the array of files includes
* a directory
Copy link
Contributor

Choose a reason for hiding this comment

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

consider adding or if this file manager does not support any of the * given paths in line with similar methods

src/java.compiler/share/classes/javax/tools/StandardJavaFileManager.java Outdated
Comment on lines 270 to 271
* elements and the given element is used by
* {@linkplain #getJavaFileObjectsFromPaths(Collection)}.
Copy link
Contributor

Choose a reason for hiding this comment

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

The new condition should be in an @implSpec.

The package declaration contains a general specification for nulls and NPE, so it's not clear that we need the @throws NPE at all, but it's not wrong to leave it as was.

@lahodaj lahodaj mentioned this pull request Jun 15, 2021
Closed
3 tasks
@lahodaj
Copy link
Contributor Author

lahodaj commented Jun 15, 2021

Transferred to JDK 17 as:
openjdk/jdk17#56

Thanks for the comments!

@lahodaj lahodaj closed this Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathan-gibbons jonathan-gibbons jonathan-gibbons requested changes

@vicente-romero-oracle vicente-romero-oracle vicente-romero-oracle approved these changes

Assignees
No one assigned
Labels
compiler compiler-dev@openjdk.org csr Pull request needs approved CSR before integration rfr Pull request is ready for review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lahodaj @jonathan-gibbons @vicente-romero-oracle