8346773: Fix unmatched brackets in some misc files #22861

MaxXSoft · 2024-12-23T09:45:00Z

This patch fixes unmatched brackets in some files, mostly in comments, docs and man pages.

Progress

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

Issue

JDK-8346773: Fix unmatched brackets in some misc files (Enhancement - P4)

Reviewers

Erik Joelsson (@erikj79 - Reviewer) 🔄 Re-review required (review applies to ec3e6aa5)
Kim Barrett (@kimbarrett - Reviewer)
Chen Liang (@liach - Reviewer) 🔄 Re-review required (review applies to ec3e6aa5)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22861

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-23T09:45:57Z

👋 Welcome back qxing! 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 · 2024-12-23T09:46:17Z

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

openjdk · 2024-12-23T09:46:53Z

@MaxXSoft The following labels will be automatically applied to this pull request:

build
client
compiler
core-libs
hotspot
i18n
serviceability

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 · 2024-12-23T09:50:10Z

Webrevs

erikj79

I can really only review the doc files and the Makefile.

erikj79 · 2024-12-23T14:00:54Z

doc/hotspot-unit-tests.md

@@ -172,7 +172,7 @@ Provide informative, but not too verbose error messages.
 All GoogleTest asserts print compared expressions and their values, so
 there is no need to have them in error messages. Asserts print only
 compared values, they do not print any of interim variables, e.g.
-`ASSERT_TRUE((val1 == val2 && isFail(foo(8)) || i == 18)` prints only
+`ASSERT_TRUE(val1 == val2 && isFail(foo(8)) || i == 18)` prints only


Looking at this expression, I believe the intention is this.

Suggested change

`ASSERT_TRUE(val1 == val2 && isFail(foo(8)) || i == 18)` prints only

`ASSERT_TRUE((val1 == val2 && isFail(foo(8))) || i == 18)` prints only

Thanks for the suggestion, updated.

justin-curtis-lu · 2024-12-23T17:11:14Z

make/data/cldr/common/main/kn.xml

@@ -1082,7 +1082,7 @@ Warnings: All cp values have U+FE0F characters removed. See /annotationsDerived/
 			<type key="calendar" type="indian">ಭಾರತೀಯ ರಾಷ್ಟ್ರೀಯ ಕ್ಯಾಲೆಂಡರ್</type>
 			<type key="calendar" type="islamic">ಇಸ್ಲಾಮಿಕ್ ಕ್ಯಾಲೆಂಡರ್</type>
 			<type key="calendar" type="islamic-civil">ಇಸ್ಲಾಮಿಕ್-ಸಿವಿಲ್ ಕ್ಯಾಲೆಂಡರ್</type>
-			<type key="calendar" type="islamic-rgsa" draft="contributed">ಇಸ್ಲಾಮಿಕ್‌ ಕ್ಯಾಲೆಂಡರ್‌ (ಸೌದಿ ಅರೇಬಿಯಾ, ಸೈಟಿಂಗ್‌)bia, sighting)</type>
+			<type key="calendar" type="islamic-rgsa" draft="contributed">ಇಸ್ಲಾಮಿಕ್‌ ಕ್ಯಾಲೆಂಡರ್‌ (ಸೌದಿ ಅರೇಬಿಯಾ, ಸೈಟಿಂಗ್‌)</type>


Hi, we would not modify the contents of upstream data from CLDR.

Sorry, I've reverted these changes in the latest commit.

kimbarrett

I strongly suggest avoiding omnibus changes like this when possible (which it
is here). Just because it's all about one "kind" of change doesn't make it a
cohesive change. This is touching many distinct areas of the JDK, and several
different languages as well. That makes it harder to review and to manage the
review, because it is large and affects a wide range of areas, so may need
many reviewers. And the whole thing might get stalled by discussion of one
piece.

There is also no mention of what testing has been done for this PR. Some of
the changes are in executable code, and need to be tested.

I think that all of the files in make/data/cldr/common are from the Unicode
Consortium, and should not be modified here.

kimbarrett · 2024-12-23T18:57:39Z

doc/hotspot-unit-tests.html

@@ -245,7 +245,7 @@ <h3 id="error-messages">Error messages</h3>
 <p>All GoogleTest asserts print compared expressions and their values,
 so there is no need to have them in error messages. Asserts print only
 compared values, they do not print any of interim variables, e.g.
-<code>ASSERT_TRUE((val1 == val2 &amp;&amp; isFail(foo(8)) || i == 18)</code>
+<code>ASSERT_TRUE(val1 == val2 &amp;&amp; isFail(foo(8)) || i == 18)</code>


This file is generated from the .md file, and should not be edited directly.

Updated both .md and .html using make update-build-docs in the latest commit.

kimbarrett · 2024-12-23T19:13:09Z

test/hotspot/jtreg/testlibrary/ctw/Makefile

@@ -47,7 +47,7 @@ LIB_FILES = $(shell find $(TESTLIBRARY_DIR)/jdk/test/lib/ \
    $(TESTLIBRARY_DIR)/jdk/test/lib/process \
    $(TESTLIBRARY_DIR)/jdk/test/lib/util \
    $(TESTLIBRARY_DIR)/jtreg \
-    -maxdepth 1 -name '*.java'))
+    -maxdepth 1 -name '*.java')


I wonder why this hasn't caused problems and been found long ago? Does make just drop that stray
close-paren?

I've tested this by running make, and the previous Makefile reports an error like file test/lib/jtreg/SkippedException.java) not found. So this fix is necessary.

I also checked the git log, this change was introduced in #22420, not that long ago.

Hmm, apparently this is not run as part of tier 1-3, which I ran in my patch. I wonder how this test is run. And it means this patch needs to be backported to 24.

Only this small piece would be appropriate for backport to 24. I suggest splitting this out into its own bug,
fixing it in isolation, and backporting that.

The purpose of this Makefile seems to be to build ctw.jar. So far as I can tell, nothing in the JDK uses that jar,
or invokes that Makefile. There are some tests for the ctw component in hotspot/jtreg/testlibrary_tests/ctw. I
think if you ran tier4 testing that you would hit those. Or you could use TEST=hotspot:hotspot_misc, or
specify that directory. But those tests don't appear to use that jar. So I think there isn't a test that touches
this Makefile.

This makefile is not used by jtreg tests. The ctw code is compiled with
@library /test/lib / /testlibrary/ctw/src
so jtreg compiled the ctw library files itself.
This makefile might be used for standalone build if needed.
Thanks for fixing this!

kimbarrett · 2024-12-23T19:25:18Z

test/jaxp/javax/xml/jaxp/unittest/transform/Bug8169112.xsl

@@ -61,8 +61,6 @@
          .landscapeArea { page: land; width: 1010px; page-break-before: always; }

          @page { margin: 0.1in; }
-
-          }


Does this affect the behavior of the test this is part of?

kimbarrett · 2024-12-23T19:30:05Z

test/jdk/sanity/client/lib/SwingSet3/src/com/sun/swingset3/demos/table/resources/oscars.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<oscars xmlns:osc-results="http://www.fatdog.com/oscar-results"


I can't tell whether anything was changed in this file other than the modification of all the end of
line indicators. I've no idea whether changing those is appropriate or not, but this file came from
a 3rd party, so might not be appropriate to change here.

+1 to all of that. This change is un-reviewable.

kimbarrett · 2024-12-23T19:30:56Z

test/jdk/sanity/client/lib/SwingSet3/src/com/sun/swingset3/demos/table/resources/oscars.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<oscars xmlns:osc-results="http://www.fatdog.com/oscar-results"


Does this change affect the behavior of the associated test(s)?

I was also wondering how this was tested.

FYI, the way to test this is ..
cd test/jdk
jtreg sanity/client/SwingSet/src/TableDemoTest.java

I tried converting all ^M to \n in this file another repo and comparing with your patch but every line was different so I just can't review this.

For safety reasons I've restored this file as well as all the files in unit tests.

kimbarrett · 2024-12-23T19:42:34Z

That makes it harder to review and to manage the review, because it is large and affects a wide range of areas, so may need many reviewers.

And many of the appropriate reviewers might be unavailable for a while, since it's right before Christmas.

prrace · 2024-12-23T20:28:07Z

/reviewers 3 reviewer

openjdk · 2024-12-23T20:28:34Z

@prrace Only the author (@MaxXSoft) is allowed to issue the reviewer command.

prrace · 2024-12-23T20:31:00Z

That makes it harder to review and to manage the review, because it is large and affects a wide range of areas, so may need many reviewers.

And many of the appropriate reviewers might be unavailable for a while, since it's right before Christmas.

I think at the very least this needs multiple reviewers. Even 2 isn't enough considering it is so cross-area.
It went to 7 different mailing lists, so that should tell you something !

erikj79 · 2024-12-24T14:54:07Z

/reviewers 2

openjdk · 2024-12-24T14:54:32Z

@erikj79
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

erikj79

I've reviewed doc/hotspot-unit-tests.* and the Makefile.

liach

Thanks for the ctw fix.

/reviewers 3 reviewer

liach · 2024-12-24T15:45:24Z

test/hotspot/jtreg/testlibrary/ctw/Makefile

@@ -47,7 +47,7 @@ LIB_FILES = $(shell find $(TESTLIBRARY_DIR)/jdk/test/lib/ \
    $(TESTLIBRARY_DIR)/jdk/test/lib/process \
    $(TESTLIBRARY_DIR)/jdk/test/lib/util \
    $(TESTLIBRARY_DIR)/jtreg \
-    -maxdepth 1 -name '*.java'))
+    -maxdepth 1 -name '*.java')


Hmm, apparently this is not run as part of tier 1-3, which I ran in my patch. I wonder how this test is run. And it means this patch needs to be backported to 24.

openjdk · 2024-12-24T15:46:40Z

@liach
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 3 Reviewers).

kimbarrett

The fix for the ctw Makefile should not be included in this PR. It's a bug, and needs
to be fixed separately and backported to 24 to fix the introduction there via the
backport of JDK-8334733 from 25 to 24, without dragging the rest of this back to 24.

MaxXSoft · 2024-12-25T02:34:30Z

@kimbarrett @liach Thanks for your suggestions!

I've moved the fix for the CTW Makefile to a separate pull request: #22878, please check it.

kimbarrett

Looks good.

AlanBateman · 2024-12-26T07:23:31Z

/label remove i18n
/label remove client

openjdk · 2024-12-26T07:24:14Z

@AlanBateman
The i18n label was successfully removed.

openjdk · 2024-12-26T07:24:39Z

@AlanBateman
The client label was successfully removed.

AlanBateman

The title on the JBS issue is a mis-leading as it's now about man pages, docs, and a comment. Looks okay.

MaxXSoft · 2024-12-26T07:44:38Z

@AlanBateman Thanks for the review! I updated the title of the JBS issue and this PR.

Fix unmatched brackets in source files.

540312c

openjdk bot added the rfr Pull request is ready for review label Dec 23, 2024

openjdk bot added serviceability [email protected] build [email protected] hotspot [email protected] client [email protected] core-libs [email protected] compiler [email protected] i18n [email protected] labels Dec 23, 2024

erikj79 reviewed Dec 23, 2024

View reviewed changes

justin-curtis-lu reviewed Dec 23, 2024

View reviewed changes

kimbarrett suggested changes Dec 23, 2024

View reviewed changes

MaxXSoft added 3 commits December 24, 2024 10:09

Do not touch upstream data from CLDR.

82e3944

Do not touch files in test.

d123ad3

Update hotspot-unit-tests.md and HTML (using Pandoc 2.19.2).

ec3e6aa

erikj79 approved these changes Dec 24, 2024

View reviewed changes

liach approved these changes Dec 24, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 24, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Dec 24, 2024

kimbarrett suggested changes Dec 24, 2024

View reviewed changes

Revert fix in the CTW Makefile.

7f85c5e

kimbarrett approved these changes Dec 25, 2024

View reviewed changes

openjdk bot removed the i18n [email protected] label Dec 26, 2024

openjdk bot removed the client [email protected] label Dec 26, 2024

AlanBateman approved these changes Dec 26, 2024

View reviewed changes

MaxXSoft changed the title ~~8346773: Fix unmatched brackets in source files~~ 8346773: Fix unmatched brackets in some misc files Dec 26, 2024

	`ASSERT_TRUE(val1 == val2 && isFail(foo(8)) \|\| i == 18)` prints only
	`ASSERT_TRUE((val1 == val2 && isFail(foo(8))) \|\| i == 18)` prints only

		<?xml version="1.0" encoding="UTF-8"?>
		<oscars xmlns:osc-results="http://www.fatdog.com/oscar-results"

8346773: Fix unmatched brackets in some misc files #22861

Are you sure you want to change the base?

8346773: Fix unmatched brackets in some misc files #22861

Conversation

MaxXSoft commented Dec 23, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 23, 2024

openjdk bot commented Dec 23, 2024 • edited Loading

openjdk bot commented Dec 23, 2024

mlbridge bot commented Dec 23, 2024 • edited Loading

Webrevs

erikj79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimbarrett commented Dec 23, 2024

prrace commented Dec 23, 2024 • edited Loading

openjdk bot commented Dec 23, 2024

prrace commented Dec 23, 2024

erikj79 commented Dec 24, 2024

openjdk bot commented Dec 24, 2024

erikj79 left a comment

Choose a reason for hiding this comment

liach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Dec 24, 2024

kimbarrett left a comment

Choose a reason for hiding this comment

MaxXSoft commented Dec 25, 2024

kimbarrett left a comment

Choose a reason for hiding this comment

AlanBateman commented Dec 26, 2024

openjdk bot commented Dec 26, 2024

openjdk bot commented Dec 26, 2024

AlanBateman left a comment

Choose a reason for hiding this comment

MaxXSoft commented Dec 26, 2024

MaxXSoft commented Dec 23, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 23, 2024 •

edited

Loading

mlbridge bot commented Dec 23, 2024 •

edited

Loading

prrace commented Dec 23, 2024 •

edited

Loading