[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-compiler-dev
Subject:    Re: RFR 8189708: class reading issue with named and annotated parameters
From:       Liam Miller-Cushon <cushon () google ! com>
Date:       2017-10-21 0:51:45
Message-ID: CAL4QsgsFpyKG3fVrwzrtpat6La_Mm8X6tm5n=P+Qwf3+q7E5MQ () mail ! gmail ! com
[Download RAW message or body]

Here's an updated patch that incorporates your approach:
http://cr.openjdk.java.net/~cushon/8007720/webrev.01/

I included the fix for JDK-8177486 so I could test the inner class / enum
constructor case. If this looks like it's on the right track I'll move that
part (and the corresponding tests) back into a separate change.

On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon <cushon@google.com>
wrote:

> Thanks for the comments,
>
> On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda <jan.lahoda@oracle.com>
> wrote:
>>
>> -what happens if there are both runtime invisible and visible annotations
>> of method's parameters? Seems those that appear later will overwrite those
>> that appear sooner?
>>
>
> Oops, thanks. The way your patch handles that looks good to me.
>
>
>> -the MethodSymbol.savedParameterAnnotations is only used during reading
>> inside the ClassReader, right? It seems wasteful to have it as a field on
>> each MethodSymbol, better would be a field in ClassReader.
>>
>
> Sounds good. I'll try to avoid having savedParameterNames as a field in
> MethodSymbol also. Do you remember if you encountered any issues with that
> in your patch?
>
>
>> -please check what happens for annotations on constructors of
>> enums/non-static innerclasses
>>
>
> Will do. (Also, note that there appears to be an issue with reading
> MethodParameters on constructors of enums/non-static inner classes:
> JDK-8177486)
>

[Attachment #3 (text/html)]

<div dir="ltr">Here&#39;s an updated patch that incorporates your approach: <a \
href="http://cr.openjdk.java.net/~cushon/8007720/webrev.01/">http://cr.openjdk.java.net/~cushon/8007720/webrev.01/</a><div><br></div><div>I \
included the fix for  <span style="font-size:12.8px">JDK-8177486 so I could test the \
inner class / enum constructor case. If this looks like it&#39;s on the right track \
I&#39;ll move that part (and the corresponding tests) back into a separate \
change.</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On \
Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon <span dir="ltr">&lt;<a \
href="mailto:cushon@google.com" target="_blank">cushon@google.com</a>&gt;</span> \
wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px \
#ccc solid;padding-left:1ex"><div dir="ltr">Thanks for the comments,<br><div \
class="gmail_extra"><br><div class="gmail_quote"><span class="">On Fri, Oct 20, 2017 \
at 12:28 AM, Jan Lahoda <span dir="ltr">&lt;<a href="mailto:jan.lahoda@oracle.com" \
class="m_-4644353677242148587gmail-cremed m_-4644353677242148587gmail-cremed \
m_-4644353677242148587cremed" target="_blank">jan.lahoda@oracle.com</a>&gt;</span> \
wrote:<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px \
                solid rgb(204,204,204);padding-left:1ex">
-what happens if there are both runtime invisible and visible annotations of \
method&#39;s parameters? Seems those that appear later will overwrite those that \
appear sooner?<br></blockquote><div><br></div></span><div>Oops, thanks. The way your \
patch handles that looks good to me.</div><span class=""><div>  </div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
                rgb(204,204,204);padding-left:1ex">
-the MethodSymbol.savedParameterAnn<wbr>otations is only used during reading inside \
the ClassReader, right? It seems wasteful to have it as a field on each MethodSymbol, \
better would be a field in \
ClassReader.<br></blockquote><div><br></div></span><div>Sounds good. I&#39;ll try to \
avoid having  <span style="color:rgb(0,0,0);white-space:pre-wrap">savedParameterNames \
as a field in MethodSymbol also. Do you remember if you encountered any issues with \
that in your patch?</span></div><span class=""><div>  </div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid \
                rgb(204,204,204);padding-left:1ex">
-please check what happens for annotations on constructors of enums/non-static \
innerclasses<br></blockquote><div><br></div></span><div>Will do. (Also, note that \
there appears to be an issue with reading MethodParameters on constructors of \
enums/non-static inner classes: JDK-8177486)</div></div></div></div> \
</blockquote></div><br></div>



[prev in list] [next in list] [prev in thread] [next in thread]