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
