On Thu, 9 Apr 2026 08:06:24 GMT, David Beaumont <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> one more assertion simplification
>
> test/jaxp/javax/xml/jaxp/unittest/stream/Bug6688002Test.java line 104:
>
>> 102: }
>> 103:
>> 104: public static/* synchronized */XMLStreamReader
>> getReader(InputStream is) throws Exception {
>
> No need for synchronization on factories.
Even worse, based on the `@summary` it sounds like the whole purpose of this
test is to verify that these methods can be called concurrently, so the `/*
synchronized */` was quite misleading?
> test/jaxp/javax/xml/jaxp/unittest/stream/StreamReaderDelegateTest.java line
> 174:
>
>> 172: }
>> 173: }
>> 174: Assert.assertTrue(hasns && hasns1);
>
> Could probably be two asserts? Or maybe refactor logic a little (it's not
> really clear what's going on here).
Even without logic refactoring, splitting the assert would probably be
beneficial?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066390798
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066400099
