RFR(T) : 8252532 : use Utils.TEST_NATIVE_PATH instead of System.getProperty("test.nativepath")
David Holmes
david.holmes at oracle.com
Mon Aug 31 04:53:09 UTC 2020
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
>
More information about the hotspot-runtime-dev
mailing list