RFR: 8257234 : Add gz option to SA jmap to write a gzipped heap dump [v9]

[Serguei Spitsyn]

On Mon, 25 Jan 2021 20:53:19 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Lin Zang has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix coding style issue
>
> 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.

Also, is it allowed to have option `gz=` without any value referring to the default value?
It seems, Chris already asked a question about max compress level limit.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1712