On Mon, 23 Dec 2024 09:45:00 GMT, Qizheng Xing <qx...@openjdk.org> wrote:
> This patch fixes unmatched brackets in some source files.
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.
doc/hotspot-unit-tests.html line 248:
> 246: so there is no need to have them in error messages. Asserts print only
> 247: compared values, they do not print any of interim variables, e.g.
> 248: <code>ASSERT_TRUE(val1 == val2 && isFail(foo(8)) || i ==
> 18)</code>
This file is generated from the .md file, and should not be edited directly.
test/hotspot/jtreg/testlibrary/ctw/Makefile line 50:
> 48: $(TESTLIBRARY_DIR)/jdk/test/lib/util \
> 49: $(TESTLIBRARY_DIR)/jtreg \
> 50: -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?
test/jaxp/javax/xml/jaxp/unittest/transform/Bug8169112.xsl line 65:
> 63: @page { margin: 0.1in; }
> 64:
> 65: }
Does this affect the behavior of the test this is part of?
test/jdk/sanity/client/lib/SwingSet3/src/com/sun/swingset3/demos/table/resources/oscars.xml
line 2:
> 1: <?xml version="1.0" encoding="UTF-8"?>
> 2: <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.
test/jdk/sanity/client/lib/SwingSet3/src/com/sun/swingset3/demos/table/resources/oscars.xml
line 2:
> 1: <?xml version="1.0" encoding="UTF-8"?>
> 2: <oscars xmlns:osc-results="http://www.fatdog.com/oscar-results";
Does this change affect the behavior of the associated test(s)?
-------------
Changes requested by kbarrett (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/22861#pullrequestreview-2521031190
PR Review Comment: https://git.openjdk.org/jdk/pull/22861#discussion_r1896040106
PR Review Comment: https://git.openjdk.org/jdk/pull/22861#discussion_r1896050113
PR Review Comment: https://git.openjdk.org/jdk/pull/22861#discussion_r1896061137
PR Review Comment: https://git.openjdk.org/jdk/pull/22861#discussion_r1896064083
PR Review Comment: https://git.openjdk.org/jdk/pull/22861#discussion_r1896064580
