Code Review for JEP 259: Stack-Walking API
Mandy Chung
mandy.chung at oracle.com
Wed Nov 11 19:38:28 UTC 2015
> On Nov 10, 2015, at 3:05 AM, Jaroslav Bachorik <jaroslav.bachorik at oracle.com> wrote:
>
> Mostly nits and typos.
>
> hotspot/src/share/vm/classfile/javaClasses.cpp
> L1941 - variable 'cpref' is not used
>
Fixed.
> hotspot/src/share/vm/classfile/javaClasses.inline.hpp
> L117 - line number 'bci + 1000000' has a special meaning?
This is existing code refactored from javaClasses.hpp. I don’t know why it added +1000000. For hidden frames, keeping line number to -1 is good to me since it means unavailable.
The runtime team may have the answer.
>
> hotspot/src/share/vm/prims/stackwalk.cpp
> L471 - typo 'pendiing' -> ‘pending'
>
Fixed.
> src/java.base/share/classes/java/lang/LiveStackFrameInfo.java
> L40,L68 - typo '""liveStackFrames"' -> '"liveStackFrames"'
> L65-66 - probably a redundant info (the exact permission is listed on L67-68
>
Fixed and the getStackWalker methods are moved to LiveStackFrame.java.
> jdk/src/java.base/share/classes/java/lang/StackWalker.java
> L222 - the documentation states only locals and operands while also monitors are to be collected
>
Fixed.
> jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java
> L478-481 - should 'initialBatchSize' be >= START_POS ?
It should be >= MIN_BATCH_SIZE. I add a check there.
> L574 - 'throw new IllegalStateException("origin " + origin + " != " + skipFrames);' -> 'throw new IllegalStateException("origin " + origin + " != " + (skipFrames + START_POS));’
>
Fixed.
> jdk/test/java/lang/StackWalker/VerifyStackTrace.java
> L66,96,130 - 'the you can' -> 'you can’
>
Fixed.
> There are several '// TODO' comments in the sources and tests - is there a plan to address them?
>
Yes. The tests are being cleaned up in the next revision.
Mandy
More information about the core-libs-dev
mailing list