JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method by justin-curtis-lu · Pull Request #16064 · openjdk/jdk

justin-curtis-lu · 2023-10-05T20:44:55Z

Please review this PR which updates ChoiceFormat and MessageFormat to no longer call overridable methods in their constructors.

The overridable methods called in the constructors are: ChoiceFormat::applyPattern, ChoiceFormat::setChoices, and MessageFormat::applyPattern. The code should be updated so that both the methods and constructors call a separate private method. Some other drive-by cleanup changes were included in the change as well.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8317630 to be approved

Issues

JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method (Bug - P4)
JDK-8317630: ChoiceFormat and MessageFormat constructors call non-final public method (CSR)

Reviewers

Naoto Sato (@naotoj - Reviewer)
Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16064/head:pull/16064
$ git checkout pull/16064

Update a local copy of the PR:
$ git checkout pull/16064
$ git pull https://git.openjdk.org/jdk.git pull/16064/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16064

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16064.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-10-05T20:46:09Z

👋 Welcome back jlu! 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 · 2023-10-05T20:48:01Z

@justin-curtis-lu The following labels will be automatically applied to this pull request:

core-libs
i18n

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.

mlbridge · 2023-10-06T17:25:35Z

Webrevs

jddarcy · 2023-10-24T04:31:36Z

I assume this issue was filed (directly or indirectly) in response to the javac lint warning added in JDK 21 under JDK-8015831.

I recommend considering how to address the underlying issue of the constructors calling overridable methods. For example, can a private method be defined that is called from the constructor and the public method?

In any case, I think one goal of bug should be to remove the warning condition, preferably by removing the condition, but, if not, by adding a @SuppressedException((this-escape").

RogerRiggs · 2023-10-24T14:08:36Z

Drive-by comment: typo "the the" in ChoiceFormat.toPattern(): line 322

openjdk · 2023-10-25T19:58:11Z

@justin-curtis-lu this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8317612
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

naotoj · 2023-10-27T18:00:38Z

src/java.base/share/classes/java/text/ChoiceFormat.java

     * @see #previousDouble
     */
-    public static final double nextDouble (double d) {
+    public static double nextDouble (double d) {


Is removing final OK here? Wouldn't this allow defining the static method in the subclass?

Right, we don't want those methods to now have the ability to be hidden. Got carried away with the IDE suggested tips. Reverted here and in the other occurrence, thanks for correcting.

naotoj · 2023-10-27T18:00:48Z

src/java.base/share/classes/java/text/ChoiceFormat.java

     * @see #nextDouble
     */
-    public static final double previousDouble (double d) {
+    public static double previousDouble (double d) {


openjdk · 2023-10-27T18:54:04Z

@justin-curtis-lu 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:

8317612: ChoiceFormat and MessageFormat constructors call non-final public method

Reviewed-by: naoto, lancea

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 315 new commits pushed to the master branch:

d226014: 8318850: Duplicate code in the LCMSImageLayout
c593f8b: 8318091: Remove empty initIDs functions
4f9f195: 8318753: hsdis binutils may place libs in lib64
2915d74: 8318837: javac generates wrong ldc instruction for dynamic constant loads
ddd0716: 8317661: [REDO] store/load order not preserved when handling memory pool due to weakly ordered memory architecture of aarch64
141dae8: 8318811: Compiler directives parser swallows a character after line comments
667cca9: 8288899: java/util/concurrent/ExecutorService/CloseTest.java failed with "InterruptedException: sleep interrupted"
b9dcd4b: 8318964: Fix build failures caused by 8315097
d52a995: 8315097: Rename createJavaProcessBuilder
957703b: 8307168: Inconsistent validation and handling of --system flag arguments
... and 305 more: https://git.openjdk.org/jdk/compare/a1c9587c27538bda3b0f6745d9c80ff4e1b9a77e...master

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 master branch, type /integrate in a new comment.

LanceAndersen

Looks reasonable. I assume you validated the links work in the generated docs as a sanity check

justin-curtis-lu · 2023-11-01T21:27:50Z

/integrate

openjdk · 2023-11-01T21:29:47Z

Going to push as commit ee57e73.
Since your change was applied there have been 364 commits pushed to the master branch:

f262f06: 8319211: Regression in LoopOverNonConstantFP
bfaf570: 8311546: Certificate name constraints improperly validated with leading period
d354141: 8318694: [JVMCI] disable can_call_java in most contexts for libjvmci compiler threads
c86592d: 8319046: Execute tests in source/class-file order in JavadocTester
3660a90: 8319139: Improve diagnosability of JavadocTester output
7f47c51: 8316025: Use testUI() method of PassFailJFrame.Builder in FileChooserSymLinkTest.java
36de19d: 8317048: VerifyError with unnamed pattern variable and more than one components
ab19348: 8318647: Serial: Refactor BlockOffsetTable
b4f5379: 8304939: os::win32::exit_process_or_thread should be marked noreturn
0461d9a: 8318016: Per-compilation memory ceiling
... and 354 more: https://git.openjdk.org/jdk/compare/a1c9587c27538bda3b0f6745d9c80ff4e1b9a77e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-01T21:29:55Z

@justin-curtis-lu Pushed as commit ee57e73.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

init

ced89d4

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Oct 5, 2023

justin-curtis-lu changed the title ~~JDK-8317630: ChoiceFormat and MessageFormat constructors call non-final public method~~ JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method Oct 5, 2023

openjdk bot added the csr Pull request needs approved CSR before integration label Oct 5, 2023

justin-curtis-lu added 2 commits October 5, 2023 15:05

Review: use 'may' wording

1f07173

code tag for MessageFormat::applyPattenr as well

b4c89dd

justin-curtis-lu marked this pull request as ready for review October 6, 2023 17:19

openjdk bot added the rfr Pull request is ready for review label Oct 6, 2023

tag setChoices as well

dc9cbae

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Oct 25, 2023

replace spec fix with private methods

2c80514

openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 26, 2023

additional cleanup/wording changes

7484712

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Oct 27, 2023

naotoj reviewed Oct 27, 2023

View reviewed changes

re-apply 'final' to the static methods in ChoiceFormat

a44ea64

naotoj approved these changes Oct 27, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 27, 2023

LanceAndersen approved these changes Oct 27, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 1, 2023

openjdk bot closed this Nov 1, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 1, 2023

Conversation

justin-curtis-lu commented Oct 5, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 5, 2023

Uh oh!

openjdk bot commented Oct 5, 2023

Uh oh!

mlbridge bot commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jddarcy commented Oct 24, 2023

Uh oh!

RogerRiggs commented Oct 24, 2023

Uh oh!

openjdk bot commented Oct 25, 2023

Uh oh!

naotoj Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justin-curtis-lu Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

naotoj Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LanceAndersen left a comment

Choose a reason for hiding this comment

Uh oh!

justin-curtis-lu commented Nov 1, 2023

Uh oh!

openjdk bot commented Nov 1, 2023

Uh oh!

openjdk bot commented Nov 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

justin-curtis-lu commented Oct 5, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 6, 2023 •

edited

Loading

naotoj Oct 27, 2023 •

edited

Loading

openjdk bot commented Oct 27, 2023 •

edited

Loading