8285727: [11u, 17u] Unify fix for JDK-8284920 with version from head

[RealCLanger]

The upstream patch for JDK-8284920 differs from the version in jdk17u-dev.
The change in head seems a bit more elegant. Also, the upstream change has a test.
So we should update jdk17u with the version from head.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 reviews required, with at least 1 reviewer)

Issue

• JDK-8285727: [11u, 17u] Unify fix for JDK-8284920 with version from head

Reviewers

• Martin Doerr (@TheRealMDoerr - Reviewer) ⚠️ Review applies to 44cc3be5
• Anton Kozlov (@AntonKozlov - no project role) ⚠️ Review applies to 44cc3be5

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17u-dev pull/373/head:pull/373
$ git checkout pull/373

Update a local copy of the PR:
$ git checkout pull/373
$ git pull https://git.openjdk.java.net/jdk17u-dev pull/373/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 373

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17u-dev/pull/373.diff

[bridgekeeper]

👋 Welcome back clanger! 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.

[mlbridge]

Webrevs

• 01: Full - Incremental (8c89cb75)
• 00: Full (44cc3be5)

[TheRealMDoerr]

@AntonKozlov Are you ok with using Oracle's fix instead? LGTM.

[AntonKozlov]

Speaking from them code itself, without considering maintainability and possible future merges from the head.

So DOT returns. I think this creates an opportunity for a mistake. IMHO, it's even will be worse, it's will be possible to write tokenIs(Token.DOT) instead of tokenIs(Token.DOT_STR), and both will be valid. But the first one peeks a single char and matches both . and .., the second one -- only .. To help to avoid such mistakes, there should be no char token that is in the prefix of a String token, such as DOT and DDOT (and now DOT_STR); COLON and DCOLON. I think making DOT a char is the step back. Instead, I would assume that COLON_CHAR should be renamed back to COLON but made also a String (to fix the last remaining char/String pair of tokens) -- but it is a bit tricky.

Worth to note the new DOT_STR is not aligned stylistically with the rest of String tokens like DDOT and DCOLON, so I don't feel the new change is elegant, but it is a matter of personal taste.

Having a test will be great.

[TheRealMDoerr]

Yeah, I think your version has advantages. Sounds like there should be a cleanup change in jdk master.

[RealCLanger]

Worth to note the new DOT_STR is not aligned stylistically with the rest of String tokens like DDOT and DCOLON, so I don't feel the new change is elegant, but it is a matter of personal taste.

OK, sorry, I shouldn't have used the word "elegant" without really looking at the implementation. What I rather wanted to point out was the fact that Oracle's change was touching less LOC.

I think, after all, we should have the same code in all OpenJDK versions. That's why I would still suggest to do this backport and additionally improve the implementation in OpenJDK head and then backport this enhancement. WDYT?

@AntonKozlov, would you mind proposing your changes in jdk/jdk?

[TheRealMDoerr]

That makes sense. This backport LGTM. Improvement in jdk (and possibly backport) would be appreciated.

[openjdk]

@RealCLanger 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:

8285727: [11u, 17u] Unify fix for JDK-8284920 with version from head

Reviewed-by: mdoerr, akozlov

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 master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

[AntonKozlov]

Yes, I think I don't need to block this. I'll take the enhancement in my queue.

[RealCLanger]

/integrate

[openjdk]

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

• c464f20: 8285726: [11u, 17u] Unify fix for JDK-8284548 with version from head
• e5a71d7: 8282929: Localized monetary symbols are not reflected in toLocalizedPattern return value
• a839a36: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc
• 75cfc99: 8284023: java.sun.awt.X11GraphicsDevice.getDoubleBufferVisuals() leaks XdbeScreenVisualInfo
• cf2b20f: 8284532: Memory leak in BitSet::BitMapFragmentTable in JFR leak profiler
• 70d3583: 8282170: JVMTI SetBreakpoint metaspace allocation test
• 0955aca: 8285394: Compiler blackholes can be eliminated due to stale ciMethod::intrinsic_id()
• db26df6: 8284549: JFR: FieldTable leaks FieldInfoTable member
• a1da138: 8284620: CodeBuffer may leak _overflow_arena

Your commit was automatically rebased without conflicts.

[openjdk]

@RealCLanger Pushed as commit a95482a.

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