RFR: 8304834: Fix wrapper insertion in TestScaffold.parseArgs(String args[]) [v4]

Chris Plummer cjplummer at openjdk.org
Tue Apr 11 17:43:41 UTC 2023


On Tue, 11 Apr 2023 16:55:42 GMT, Leonid Mesnik <lmesnik at 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


More information about the serviceability-dev mailing list