On Tue, 11 Apr 2023 16:55:42 GMT, Leonid Mesnik <lmes...@openjdk.org> wrote:
>> The TestScaffold incorrectly parse options, it should insert wrapper class >> between VM options and applications classame. > > Leonid Mesnik has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains eight commits: > > - updated after David's comments. > - Merge branch 'master' of https://github.com/openjdk/jdk into 8289546 > - updated parsing. > - added comments and trim arguments > - parsing updted > - problemlist fixed > - comments > - fix Changes requested by cjplummer (Reviewer). test/jdk/com/sun/jdi/TestScaffold.java line 471: > 469: // Parse arguments, like java/j* tools command-line arguments > 470: // the first argument not-starting with '-' is treated as a > classname > 471: // the other arguments are split to targetVMArgs > targetAppCommandLine correspondingly Can you make these proper sentences with the first letter uppercase and a period at the end. test/jdk/com/sun/jdi/TestScaffold.java line 475: > 473: // The result without any wrapper enabled: > 474: // argInfo.targetAppCommandLine : Frames2Targ > 475: // argInfo.targetVMArgs : -Xss4M The 2 argInfo comments would be easier to read if indented 2 or 4 spaces (after the //, not before). test/jdk/com/sun/jdi/TestScaffold.java line 478: > 476: // The result with wrapper enabled: > 477: // argInfo.targetAppCommandLine : TestScaffold Virtual > Frames2Targ > 478: // argInfo.targetVMArgs : -Xss4M The 2 argInfo comments would be easier to read if indented 2 or 4 spaces (after the //, not before). test/jdk/com/sun/jdi/TestScaffold.java line 500: > 498: redefineAsynchronously = true; > 499: } else if (arg.startsWith("-J")) { > 500: throw new RuntimeException("-J-option format is not > supported. Incorrect arg: " + args[i]); David pointed out that you can use arg instead of args[i] here. ------------- PR Review: https://git.openjdk.org/jdk/pull/13170#pullrequestreview-1379784054 PR Review Comment: https://git.openjdk.org/jdk/pull/13170#discussion_r1163138251 PR Review Comment: https://git.openjdk.org/jdk/pull/13170#discussion_r1163138841 PR Review Comment: https://git.openjdk.org/jdk/pull/13170#discussion_r1163138984 PR Review Comment: https://git.openjdk.org/jdk/pull/13170#discussion_r1163137029
