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

[Igor Ignatyev]

thanks Serguei, David, pushed.

-- Igor

> Hi Igor,
> 
> The update looks good.
> 
> Thanks,
> Serguei


> On Sep 1, 2020, at 4:58 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> 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
>>>