Code Review for JEP 259: Stack-Walking API

[Brent Christian]

Hi, Mandy

I looked through the jdk code.  I think it's looking quite good.  I just 
noticed some small details to consider fixing up before pushing.

Docs:

StackWalker.StackFrame.getClassName():
the "binary name" link seems to be broken
(StackWalker.java line 100)

StackWalker.getInstance(Set<StackWalker.Option> options):
StackWalker.getInstance(Set<StackWalker.Option> options, int estimateDepth):

A couple missing words:

"Returns a StackWalker instance with the given *options* specifying..."

"If the given *["options"|"set"]* is empty, ..."

(Looks like a typo ("@ocde") on StackWalker.java lines 278 & 307)


Code:

==
jdk/src/java.base/share/classes/sun/util/logging/PlatformLogger.java

550:
The comment for get() states that it returns a StackTraceElement

==
hotspot/src/share/vm/prims/jvm.h (219)
jdk/src/java.base/share/native/include/jvm.h (196)

FWIW, the start_index argument of
JVM_FillStackFrames is an int in the hotspot file, and a jint in the jdk 
file.

==
jdk/src/java.logging/share/classes/java/util/logging/LogRecord.java

658:
The comment for get() states that it returns a StackTraceElement

==
jdk/src/java.base/share/classes/java/lang/StackStreamFactory.java

  137         // VM writes to these arrays to fill the stack frame 
information
  138         // for each batch

I think this comment applies to a previous version of the code.


  195          * Subclass should override this method to change the 
batch size
  196          * or invoke the function passed to the StackWalker::walk 
method

I believe the function in question is the batchSizeMapper function, no 
longer being passed to walk(); line 196 can be removed.

==
jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java

66
Perhaps this.memberName does not need to be allocated in the case of 
-XX:-MemberNameInStackFrame ?

Thanks,
-Brent

On 11/16/15 4:13 PM, Mandy Chung wrote:
> I’d like to get the code review done by this week.
>
> I renamed the static factory method from create to getInstance since “create” implies to create a new instance but the method returns a cached instance instead.  I also changed the spec of getCallerClass per [1].  There is not much change since webrev.01.
>
> Webrev:
>     http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02
>
> javadoc:
>     http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/
>
> Mandy
> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036589.html
>