8377910: Minor cleanup of java/io/FileDescriptor/Sharing.java#29718
8377910: Minor cleanup of java/io/FileDescriptor/Sharing.java#29718bplb wants to merge 3 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back bpb! A progress list of the required criteria for merging this PR into |
|
It appears that the |
|
@bplb 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 13 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
| if(fisArray[i] != null) fisArray[i].close(); | ||
| if(fosArray[i] != null) fosArray[i].close(); | ||
| fisArray[i].close(); | ||
| fosArray[i].close(); |
There was a problem hiding this comment.
It might be clearer if you change fisArray and fosArray to be local to the run method. There is no reason for them to be fields in OpenClose. Making fd and done final would help too.
I can't see from the bug report what failed but hopefully better output with the changes.
There was a problem hiding this comment.
I can't see from the bug report what failed but hopefully better output with the changes.
It's not clear exactly what failed (in the parent task issue) but perhaps this will disambiguate the situation a bit.
There was a problem hiding this comment.
It might be clearer if you change fisArray and fosArray to be local to the run method. There is no reason for them to be fields in OpenClose. Making fd and done final would help too.
Done in 9dd1613.
Note that this code has bad indentation but I am leaving that alone until the end.
There was a problem hiding this comment.
Note that this code has bad indentation but I am leaving that alone until the end.
Indentation fixed in 73d9eab.
… instance variables final
| if (fd.valid()) { // fd should not be valid after first close() call | ||
| System.out.println("OpenClose: FileDescriptor shouldn't be valid"); | ||
| System.err.println("OpenClose: FileDescriptor shouldn't be valid"); | ||
| fail = true; |
There was a problem hiding this comment.
The failure reported in the parent issue occurs because fd.valid() is true at line 378. Code inspection alone does not reveal any reason for it.
2 participants
Null checks potentially mask a more serious, fundamental problem and should be removed to allow a
NullPointerExceptionto be throw if the variable isnull.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/29718/head:pull/29718$ git checkout pull/29718Update a local copy of the PR:
$ git checkout pull/29718$ git pull https://git.openjdk.org/jdk.git pull/29718/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 29718View PR using the GUI difftool:
$ git pr show -t 29718Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/29718.diff
Using Webrev
Link to Webrev Comment