Hi Mandy,
This looks good to me.
Regarding final methods, I believe it might be prudent
to make toStackTraceElement() final - and possibly also
all the getters that call it.
It would be imprudent to try to override any of these
methods in a subclass without looking at the implementation
in the superclass. Making these methods final will ensure
that future maintainers will at least need to look at
the implementation in StackFrameInfo before they
override any of them.
best regards,
-- daniel
On 28/04/16 21:42, Mandy Chung wrote:
On Apr 28, 2016, at 11:37 AM, Brent Christian <brent.christ...@oracle.com>
wrote:
Hi, Mandy
It looks good to me. A few minor things:
StackFrameInfo.java:
Should getByteCodeIndex() be final ?
It’s a package-private class and so no subclass outside this package anyway.
So it doesn’t really matter. I didn’t catch the inconsistency there - some
methods in this class are final and some are not. I may clean them up.
StackWalker.java, line 136:
* change ';' to ‘,'
ok.
JavaLangInvokeAccess.java, line 40:
For the isNative() docs, I would prefer "Returns true if..." to "Tests if..."
Both conventions are used. I can change that.
Some test code for getByteCodeIndex() would be good - sounds like you plan to
add that.
yes. Will send out for review next.
thanks for the review.
Mandy
Thanks,
-Brent
On 04/27/2016 11:31 AM, Mandy Chung wrote:
Updated webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153912/webrev.01/index.html
I added a new StackFrame::getByteCodeIndex method to return bci and
updatedgetFileName and getLineNumber to have the same returned type as in
StackTraceElement. From usage perspective, the caller has to prepare and
handle the information is unavailable and I think it would typically log some
token to represent the unavailable case and might well use null and negative
value. This patch would save the creation of short-lived Optional object that
might help logging filename/linenumber case.
I’m working on a new test to verify bci and line number to be included in the
next revision.
Mandy
On Apr 11, 2016, at 2:22 PM, Mandy Chung <mandy.ch...@oracle.com> wrote:
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8153912/webrev.00/index.html
StackFrame::getFileName and StackFrame::getLineNumber are originally proposed
with the view of any stack walking code can migrate to the StackWalker API
without the use of StackTraceElement.
File name and line number are useful for debugging and troubleshooting purpose.
It has additional overhead to map from a method and BCI to look up the file
name and line number.
StackFrame::toStackTraceElement method returns StackTraceElement that includes
the file name and line number. There is no particular benefit to duplicate
getFileName and getLineNumber methods in StackFrame. It is equivalently
convenient to call StackFrame.toStackTraceElement().getFileName() (or
getLineNumber).
This patch proposes to remove StackFrame::getFileName and
StackFrame::getLineNumber methods since such information can be obtained from
StackFrame.toStackTraceElement().
Mandy
