RFR(T) : 8252532 : use Utils.TEST_NATIVE_PATH instead of System.getProperty("test.nativepath")

[David Holmes]

Hi Igor,

Changes seem fine (incremental webrev didn't show them all though).

There are other pre-existing indentation issues in those files but this 
is fine.

Thanks,
David

On 31/08/2020 2:53 pm, David Holmes wrote:
> Hi Igor,
> 
> On 29/08/2020 1:52 pm, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8252532/webrev.00
>>> 145 lines changed: 28 ins; 22 del; 95 mod;
>>
>>
>> Hi all,
>>
>> could you please review this trivial clean up which replaces 
>> System.getProperty("test.nativepath") w/ Utils.TEST_NATIVE_PATH where 
>> appropriate?
>>
>> while updating these files, I've also cleaned them up a bit, removed 
>> unneeded imports, added/removed spaces, etc
>>
>> testing: runtime, serviceability and vmTestbase/nsk/jvmti/ tests on 
>> {linux,windows,macos}-x64
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8252532
>> webrev: http://cr.openjdk.java.net/~iignatyev//8252532/webrev.00
> 
> Generally seems fine (though the fact the patch file contained a series 
> of changesets threw me initially!)
> 
> test/hotspot/jtreg/runtime/signal/SigTestDriver.java
> 
>           // add test specific arguments w/o signame
>           cmd.addAll(Arrays.asList(args)
> -                         .subList(1, args.length));
> +                .subList(1, args.length));
> 
> Your changed line doesn't have the right indent. Can this just be put on 
> one line anyway:
> 
>           // add test specific arguments w/o signame
>           cmd.addAll(Arrays.asList(args).subList(1, args.length));
> 
> that seems better to me as the fact there is only one argument seems 
> clearer. Though for greater clarity perhaps:
> 
>           // add test specific arguments w/o signame
>           var argList = Arrays.asList(args).subList(1, args.length);
>           cmd.addAll(argList);
> 
> -- 
> 
> +                Arrays.stream(Utils.JAVA_OPTIONS.split(" ")))
> +                .filter(s -> !s.isEmpty())
> +                .filter(s -> s.startsWith("-X"))
> +                .flatMap(arg -> Stream.of("-vmopt", arg))
> +                .collect(Collectors.toList());
> 
> The preferred/common style for chained stream operations is to align the 
> dots:
> 
>            Arrays.stream(Utils.JAVA_OPTIONS.split(" ")))
>                  .filter(s -> !s.isEmpty())
>                  .filter(s -> s.startsWith("-X"))
>                  .flatMap(arg -> Stream.of("-vmopt", arg))
>                  .collect(Collectors.toList());
> 
> ---
> 
> test/lib/jdk/test/lib/process/ProcessTools.java
> 
> -        System.out.println("\t" +  t +
> -                           " stack: (length = " + stack.length + ")");
> +        System.out.println("\t" + t +
> +                " stack: (length = " + stack.length + ")");
> 
> The original code is more stylistically correct - when breaking 
> arguments across lines the indent should align with the start of the 
> arguments.
> 
> Similarly here:
> 
> +        return String.format("--- ProcessLog ---%n" +
> +                        "cmd: %s%n" +
> +                        "exitvalue: %s%n" +
> +                        "stderr: %s%n" +
> +                        "stdout: %s%n",
> +                getCommandLine(pb), exitValue, stderr, stdout);
> 
> should be:
> 
> +        return String.format("--- ProcessLog ---%n" +
> +                             "cmd: %s%n" +
> +                             "exitvalue: %s%n" +
> +                             "stderr: %s%n" +
> +                             "stdout: %s%n",
> +                             getCommandLine(pb), exitValue, stderr, 
> stdout);
> 
> and here:
> 
> +        String executable = Paths.get(Utils.TEST_NATIVE_PATH, 
> executableName)
> +                .toAbsolutePath()
> +                .toString();
> 
> indentation again.
> 
> Thanks,
> David
> -----
> 
>> Thanks,
>> -- Igor
>>