On Thu, 15 Jul 2021 17:22:31 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Lin Zang has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains five additional commits
>> since the last revision:
>>
>> - fix indentation issue
>> - Merge branch 'master' into try
>> - revise code to handle the closing of embeded streams
>> - Merge branch 'master' into try
>> - 8269909: getStack method in hprof.parser.Reader should use
>> try-with-resource
>
> Hi Lin,
> These local names with extra numbers look strange.
> You introduced these numbers in order to fix naming conflicts.
> You also can avoid these conflicts by refactoring the code.
> Some of these fragments can be refactored to become a separate methods.
> I do not want to push hard on you with this but it is just something to
> consider to simplify the code and avoid such naming problems.
> Thanks,
> Serguei
Dear Serguei(@sspitsyn ),
> Some of these fragments can be refactored to become a separate methods.
I have tried to extract the common codes to a seperate method :
HprofReader getHprofReader() {
try (FileInputStream outFis = new FileInputStream(out);
BufferedInputStream outBis = new BufferedInputStream(outFis);
PositionDataInputStream pdin = new PositionDataInputStream(outBis)) {
i = pdin.readInt();
if (i == HprofReader.MAGIC_NUMBER) {
HprofReader r
= new HprofReader(deCompressedFile, pdin, dumpNumber,
true, debugLevel);
return r;
}
and then call:
HprofReader r = getHprofReader();
r.read(); // or r.printStackTraces();
But it doesn't work because the streams are closed when method `getHprofReader`
returns, and the `r.read()` or `r.printStackTraces()` still need to read data
from the stream, which throws exception.
Moreover, I also found that the method `getStack()` and `readFile()` have
almost the same logic. So maybe there is a chance that they could be combined
together, or one the them could be omit if only used for testing purpose (may
also need to adjust the related heap dump test cases). And IMHO the
`GzipRandomAccess` class may not even be necessary then.
However, I think it is not quite relate to this PR. So I just update the patch
that only updated the variable name. and I suggest to use a new issue for code
refecting in future.
What do you think?
Thanks!
Lin
-------------
PR: https://git.openjdk.java.net/jdk/pull/4717
