-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8268698: Use Objects.check{Index,FromToIndex,FromIndexSize} for java.base #4507
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
Conversation
|
👋 Welcome back yyang! A progress list of the required criteria for merging this PR into |
|
@kelthuzadx The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
|
/label remove 2d |
|
@kelthuzadx |
|
@kelthuzadx |
|
@kelthuzadx |
|
@kelthuzadx |
|
@kelthuzadx |
|
@kelthuzadx |
|
@kelthuzadx |
|
@kelthuzadx I did not see any response to my query about the change in initialization (not load) order. I also remain concerned about introducing cross dependencies between core classes (e.g. String now uses Precondition, so does that impact Precondition using String?) But these are things for the core-libs folk to be concerned about, or not. Cheers, |
Hi David, the following are initialization orders: It looks this patch will not affect the initialization order. As far as I observed, the only problem caused by cross dependencies between core classes has been discussed above. I think it would be ok, I'd like to have more comments from core-lib folks about this. |
mlchung
left a comment
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 suggest to separate the client changes (both src and test) in a separate PR for the client review.
I review the rest of the files, which looks okay to me. I will take another pass carefully.
src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java needs to be updated in JSR 166 upstream repo. Better to file a separate issue for this change to ensure that gets fixed in the upstream project.
| public void run() { | ||
| new StringBuffer(); | ||
| } | ||
| }); |
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.
Nit: The above formatting (line 70-97) is inconsistent with the formatting in line 110-124. It'd be good to use the same formatting.
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.
Hi Mandy, thanks for reviewing this.
I suggest to separate the client changes (both src and test) in a separate PR for the client review.
Does "client changes" means changes involving src/java.desktop and test/java/awt?
src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java needs to be updated in JSR 166 upstream repo. Better to file a separate issue for this change to ensure that gets fixed in the upstream project.
Can you please paste a link for that? I'm not sure where I can find JSR 166 upstream repo..
Nit: The above formatting (line 70-97) is inconsistent with the formatting in line 110-124. It'd be good to use the same formatting.
Restored.
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.
Does "client changes" means changes involving src/java.desktop and test/java/awt?
src/java.desktop, test/java/awt, and test/javax/imageio
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.
src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java needs to be updated in JSR 166 upstream repo. Better to file a separate issue for this change to ensure that gets fixed in the upstream project.
Can you please paste a link for that? I'm not sure where I can find JSR 166 upstream repo..
What I meant is to file a JBS issue for this change and revert the change in this patch. That can be fixed when the next JSR 166 changes are imported to JDK.
I wasn't sure if this is the right repo: http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/
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.
Okay. I've filed https://bugs.openjdk.java.net/browse/JDK-8270057 and https://bugs.openjdk.java.net/browse/JDK-8270058 for JSR 166 and client code respectively. These codes have been restored.
(Sorry for force-pushing, my mistake..)
| public void run() { | ||
| new StringBuilder(); | ||
| } | ||
| }); |
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.
Nit: it'd be good to make the formatting of all calls to tryCatch method consistent.
Alan and Paul are currently on vacation. I'm reviewing it. |
mlchung
left a comment
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.
Looks good. Thanks for separating the other files from this patch. I also did a test run on tier1-3 that looks clean.
|
@kelthuzadx 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 383 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 |
|
I'm not familiar with the review process of core-lib group. Is it sufficient for merging with one approval? Should I have more reviews for this? 🤔 |
It depends on the change. For this patch, it's good to get another reviewer to look through it. |
RogerRiggs
left a comment
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.
Looks good.
|
Thanks Mandy and Roger for reviews! /integrate |
|
Going to push as commit afe957c.
Your commit was automatically rebased without conflicts. |
|
@kelthuzadx Pushed as commit afe957c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
After JDK-8265518(#3615), it's possible to replace all variants of checkIndex by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in the whole JDK codebase.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507$ git checkout pull/4507Update a local copy of the PR:
$ git checkout pull/4507$ git pull https://git.openjdk.java.net/jdk pull/4507/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4507View PR using the GUI difftool:
$ git pr show -t 4507Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4507.diff