> On Feb 7, 2017, at 5:10 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
>
> Hi Mandy,
>
> On 06/02/17 00:16, Mandy Chung wrote:
>> Hi Daniel,
>>
>> Thanks for the patch and uncover that Constructor::newInstance is not
>> covered by SHOW_REFLECT_FRAMES.
>>
>> The change looks okay. The javadoc of SHOW_REFLECT_FRAMES can be clarified
>> to include Constructor::newInstance. As for the test, can you separate this
>> in a new test to test showing and hiding reflecton frames and also
>> getCallerClass?
>
> Please find below a new webrev that incorporates your feedback.
> I have added a new test, and also tweaked the API documentation
> in StackWalker.java as you suggested:
> http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.03/
>
I think this sentence is not needed.
216 * <p>A {@code StackWalker} with this {@code SHOW_REFLECT_FRAMES}
option
217 * will show all reflection frames.
Basic.java test - the @bug already includes the bug number. line 67-69 can be
removed.
Minor comments: StackFrame.getClassName() can replace
StackFrame.getDeclaringClass().getName(). Or if you want to compare package
name, StackFrame.getDeclaringClass().getPackageName is an alternative.
Nit: what about renaming SimpleObj to ConstructorNewInstance?
SimpleObj::found should probably name as `framesInUnnamedOrReflectionPackage`.
Nit: ReflectionFrames.java test - the test is a bit hard maybe SimpleObj can be
renamed to TestHelper and `found` be `result`. Just trying to make the test
easier to read and understand. You may have better idea.
Otherwise looks good.
Mandy
