[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(T) : 8252532 : use Utils.TEST_NATIVE_PATH instead of System.getProperty("test.nativepath")
From:       David Holmes <david.holmes () oracle ! com>
Date:       2020-08-31 4:53:09
Message-ID: 93076ed6-f112-dd27-15d3-13f67cdf5de0 () oracle ! com
[Download RAW message or body]

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
> 


[prev in list] [next in list] [prev in thread] [next in thread]