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

[Igor Ignatyev]

Hi Serguei, David,

thanks for your reviews, I've updated the patch to address David's comments:
 http://cr.openjdk.java.net/~iignatyev//8252532/webrev.01/ <http://cr.openjdk.java.net/~iignatyev//8252532/webrev.01/> (whole)
 http://cr.openjdk.java.net/~iignatyev//8252532/webrev.0-1/ <http://cr.openjdk.java.net/~iignatyev//8252532/webrev.0-1/> (incremental)

Thanks,
-- Igor

> On Sep 1, 2020, at 1:39 AM, serguei.spitsyn at oracle.com wrote:
> 
> 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);
>> 
agree
>> -- 
>> 
>> +                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());
>> 
`collect` is actually called on the result of `Stream.concat`, anyhow I've aligned the chained calls by dots.
>> ---
>> 
>> 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 + ")");
I've decided to put this on one line. 
>> 
>> 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);
fixed
>> 
>> and here:
>> 
>> +        String executable = Paths.get(Utils.TEST_NATIVE_PATH, executableName)
>> +                .toAbsolutePath()
>> +                .toString();
fixed
>> 
>> indentation again.
>> 
>> Thanks,
>> David
>> -----
>> 
>>> Thanks,
>>> -- Igor
>>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200901/44c3d692/attachment.htm>