On Tue, 9 Feb 2021 06:37:21 GMT, Lin Zang <lz...@openjdk.org> wrote:
>> Hi Lin,
>>
>> Sorry for confusing you but I've not noticed a duplication in your code:
>>
>> 1778 if (cntTokens > 2) {
>> 1779 usage();
>> 1780 } else {
>> 1781 JMap jmap = new JMap();
>> 1782 String filename = "heap.bin";
>> 1783 int gzlevel = 0;
>> . . . .
>> 1794 if (cntTokens > 2) {
>> 1795 err.println("More than 2 options specified: " +
>> cntTokens);
>> 1796 usage();
>> 1797 return;
>> 1798 }
>> 1799 if (cntTokens == 1) { // first argument could be
>> filename or "gz="
>> . . . .
>>
>> There are two checks for cntTokens > 2. The second check has to be removed.
>> Also, a return needs to be added after the line 1779 with "usage();" , so
>> the "else" statement can be removed.
>> This has to be refactored to something like this:
>> 1778 if (cntTokens > 2) {
>> err.println("More than 2 options specified: " +
>> cntTokens);
>> 1779 usage();
>> return;
>> 1780 }
>> 1781 JMap jmap = new JMap();
>> 1782 String filename = "heap.bin";
>> 1783 int gzlevel = 0;
>> 1799 if (cntTokens == 1) { // first argument could be
>> filename or "gz="
>> . . . .
>>
>> Thanks,
>> Serguei
>
> Dear Serguei,
>
>> There are two checks for cntTokens > 2. The second check has to be removed.
>> Also, a return needs to be added after the line 1779 with "usage();" , so
>> the "else" statement can be removed.
>> This has to be refactored to something like this:
>
> Nice catch and good suggestion. Thanks a lot for point it out!
> I think It is no hurry to push this PR, and welcome for any comment.
> Thanks!
Hi Lin,
Thank you for update.
test/lib/jdk/test/lib/hprof/parser/Reader.java:
+ } else if ((i >>> 8) == 0x1f8b08) {
+ // Possible gziped file.
Could you, please, define a named constant and use it instead of the literal
value 0x1f8b08?
Also, would it make sense to extend the comment to explain about it a little
bit?
I'm still looking at some files (e.g., HeapHprofBinWriter.java).
Thanks,
Serguei
-------------
PR: https://git.openjdk.java.net/jdk/pull/1712
