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

[serguei.spitsyn at oracle.com]

Hi Igor,

This looks fine to me too.
I also agree with David's suggestions.

Thanks,
Serguei


On 8/30/20 21:53, 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
>>