> On Feb 8, 2017, at 3:36 AM, Daniel Fuchs <daniel.fu...@oracle.com> wrote:
>
> Hi Mandy,
>
> Thanks for all the suggestions!
>
> On 07/02/17 20:03, Mandy Chung wrote:
>>> Please find below a new webrev that incorporates your feedback.
>>> 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.
>
> I disagree: the text starts by saying:
> "By default reflection frames are hidden. The reflection
> frames include […]"
209 * Shows all reflection frames.
This is the first sentence. line 216-217 repeats line 209.
> but the sentence at lines 216-217 is the only place where we say what
> the option actually does: it shows all reflection frames.
> I think it's needed.
> Possibly the sentence at lines 216-217 could be moved to become
> the first sentence in the doc comment, but I'd rather not change
> it now that the CCC has approved the doc changes as in webrev.03.
This does not change the spec and this is minor rewording. No need to update
CCC.
What about:
Shows all reflection frames.
<p>By default, reflection frames are hidden. A {@code StackWalker} with this
{@code SHOW_REFLECT_FRAMES} option will show the reflection frames that include
….
>
>
>> Nit: what about renaming SimpleObj to ConstructorNewInstance?
>> SimpleObj::found should probably name as
>> `framesInUnnamedOrReflectionPackage`.
>
> SimpleObj => ConstructorNewInstance
> found => testFramesOrReflectionFrames
> (frames in the unnamed package are test frames)
>
> I added some comments as well in the accept() method.
That’s good.
>
>> 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.
>
> SimpleObj => StackInspector (it calls walk & getCallerClass
> to inspect the stack from its constructor)
> found => collectedFrames
>
> Also added some comments to better explain the purposes of
> StackInspector and StackInspector.Caller.
>
> Here is the new webrev:
> http://cr.openjdk.java.net/~dfuchs/webrev_8173898/webrev.04/
>
Thanks for updating the test.
Mandy
