On Tue, 26 Jan 2021 08:50:06 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java
>> line 2119:
>>
>>> 2117: }
>>> 2118: } else {
>>> 2119: err.println("Unknow option \"" + option + "\"");
>>
>> "Unknown"
>
> - if (t.countTokens() > 1) {
> + if (t.countTokens() > 2) {
> usage();
> } else {
> JMap jmap = new JMap();
> - String filename;
> - if (t.countTokens() == 1) {
> - filename = t.nextToken();
> - } else {
> - filename = "heap.bin";;
> + String filename = "heap.bin";
> + int gzlevel = -1;
> + int cntTokens = t.countTokens();
> I'd suggest to move the last line above to the beginning of method and use it
> in first comparison as well to make it more consistent.
>
> It'd be nice to make the comments more consistent.
> For instance:
> `+ * possible command:`
> Start it from capital letter: `Possible`.
>
> Replace:
> + /* first argument is compression level, second
> is filename */
> + /* Parse "gz=" option. */
> with:
> + /* First argument is compression level, second
> is filename. */
> + * Parse "gz=" option. */
>
> Replace:
> `+ // Try to parse "gz=" option, if failed, treat
> it as filename`
> with:
> `+ // Try to parse "gz=" option. If failed, treat
> it as filename.`
>
> Start from capital letter:
> `+ // compression level not in range.`
>
>
> It'd be nice to reduce the following:
> + if (cntTokens > 0) {
> + String option = t.nextToken();
> + /*
> + * possible command:
> + * dumpheap gz=1 file;
> + * dumpheap gz=1;
> + * dumpheap file;
> + */
>
> to:
> + String option = t.nextToken();
> + /*
> + * Possible command:
> + * dumpheap gz=1 file;
> + * dumpheap gz=1;
> + * dumpheap file;
> + * dumpheap;
> + */
>
> It does not seem the condition `if (cntTokens > 0) {` is very useful.
>
> The checks below seems to be incorrect:
> + **if (gzlevel < 1)** {
> + err.println("Can not parse compression
> level from option "" + option + "".");
> + **if (gzlevel == 0)** {
> + // compression level not in range.
> + usage();
> + return;
> + } else {
> + out.println("Use "" + option +"" as
> dumped file name.");
> + }
> + } else {
> + filename = "heap.bin.gz";
> + }
>
> At least, how the gzlevel can be equal to 0 if it was already fount it is
> below 0?
>
> It seems, Chris has already asked a question about checking the max compress
> level limit.
Hi @sspitsyn,
> It does not seem the condition if (cntTokens > 0) { is very useful.
This is used to guard the case cntTokens == 0, otherwise that t.nextToken()
will throw ArrayIndexOutOfBoundsException.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1712
