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

[serguei.spit...@oracle.com]

Hi Igor,

The update looks good.

Thanks,
Serguei


On 9/1/20 13:21, Igor Ignatyev wrote:

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/ (whole)

 http://cr.openjdk.java.net/~iignatyev//8252532/webrev.0-1/ (incremental)

Thanks,

-- Igor

On Sep 1, 2020, at 1:39 AM, serguei.spit...@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