On Fri, 17 May 2024 19:45:00 GMT, Calvin Cheung <cche...@openjdk.org> wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> (1) comments from @liach; (2) added javadoc; (3) Use
>> createTestJavaProcessBuilder() instead of
>> createLimitedTestJavaProcessBuilder()
>
> test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBench.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights
>> reserved.
>
> Since this is a new file, should the copyright year be only 2024?
> (same for other files)
The file was first added in the Leyden repo in 2023, so I think we should use
that as the initial copyright year.
> test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBench.java line 43:
>
>> 41: import jdk.test.lib.cds.CDSAppTester;
>> 42: import jdk.test.lib.helpers.ClassFileInstaller;
>> 43: import jdk.test.lib.process.OutputAnalyzer;
>
> Is this import needed?
Removed.
> test/hotspot/jtreg/runtime/cds/appcds/applications/JavacBenchApp.java line
> 222:
>
>> 220: }
>> 221: long elapsed = System.currentTimeMillis() - started;
>> 222: if (System.getProperty("JavacBenchApp.silent") == null) {
>
> Should line 221 be inside the 'if' block?
I moved it inside the 'if' block.
> test/lib/jdk/test/lib/cds/CDSAppTester.java line 218:
>
>> 216: }
>> 217:
>> 218: throw new RuntimeException("Must have exactly one command line
>> argument: STATIC or DYNAMIC");
>
> Why not check the number of args at the beginning of the method?
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1607047769
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1607047882
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1607047859
PR Review Comment: https://git.openjdk.org/jdk/pull/19256#discussion_r1607047805
