thanks Serguei, David, pushed. -- Igor
> Hi Igor, > > The update looks good. > > Thanks, > Serguei > On Sep 1, 2020, at 4:58 PM, David Holmes <[email protected]> wrote: > > Hi Igor, > > Changes seem fine (incremental webrev didn't show them all though). > > There are other pre-existing indentation issues in those files but this is > fine. > > Thanks, > David > > On 31/08/2020 2:53 pm, 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); >> -- >> + 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 >>>
