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, [email protected] 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 >>> >
