<div dir="ltr"><div>Thanks,</div><div><br></div><div>I updated the copyright header and moved the tests to the correct location:<br></div><div><a href="http://cr.openjdk.java.net/~cushon/8007720/webrev.02/">http://cr.openjdk.java.net/~cushon/8007720/webrev.02/</a><br></div><div><br></div><div>Also - note that the patch currently includes a fix for JDK-8177486 as well, since you suggested I add test coverage for enum and inner class constructors, and mandated parameters aren't handled correctly without fixing the other bug. Is it OK to have one changeset for both issues, or should they be separated out? I could remove the enum and inner class test coverage and the fix, and then add it back in as a follow-up change if you prefer.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 13, 2017 at 8:01 AM, Jan Lahoda <span dir="ltr"><<a href="mailto:jan.lahoda@oracle.com" target="_blank">jan.lahoda@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Sorry for late reply.<br>
<br>
Seems OK to me, only:<br>
-the tests should be in:<br>
test/langtools/tools/javac/Met<wbr>hodParameters/8189708<br>
not in:<br>
test/tools/javac/MethodParamet<wbr>ers/8189708<br>
<br>
-test/tools/javac/MethodParame<wbr>ters/8189708/Test.java has a weird license header<br>
<br>
Vicente, could you please take a look as well?<br>
<br>
Thanks,<br>
    Jan<span class=""><br>
<br>
On 4.12.2017 22:37, Liam Miller-Cushon wrote:<br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
Hi, is there any more feedback on this?<br>
<br>
On Fri, Oct 20, 2017 at 5:51 PM, Liam Miller-Cushon <<a href="mailto:cushon@google.com" target="_blank">cushon@google.com</a><br></span><span class="">
<mailto:<a href="mailto:cushon@google.com" target="_blank">cushon@google.com</a>>> wrote:<br>
<br>
    Here's an updated patch that incorporates your approach:<br>
    <a href="http://cr.openjdk.java.net/~cushon/8007720/webrev.01/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~cu<wbr>shon/8007720/webrev.01/</a><br>
    <<a href="http://cr.openjdk.java.net/~cushon/8007720/webrev.01/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~c<wbr>ushon/8007720/webrev.01/</a>><br>
<br>
    I included the fix for JDK-8177486 so I could test the inner class /<br>
    enum constructor case. If this looks like it's on the right track<br>
    I'll move that part (and the corresponding tests) back into a<br>
    separate change.<br>
<br>
    On Fri, Oct 20, 2017 at 9:30 AM, Liam Miller-Cushon<br></span><span class="">
    <<a href="mailto:cushon@google.com" target="_blank">cushon@google.com</a> <mailto:<a href="mailto:cushon@google.com" target="_blank">cushon@google.com</a>>> wrote:<br>
<br>
        Thanks for the comments,<br>
<br>
        On Fri, Oct 20, 2017 at 12:28 AM, Jan Lahoda<br></span><span class="">
        <<a href="mailto:jan.lahoda@oracle.com" target="_blank">jan.lahoda@oracle.com</a> <mailto:<a href="mailto:jan.lahoda@oracle.com" target="_blank">jan.lahoda@oracle.com</a>><wbr>> wrote:<br>
<br>
            -what happens if there are both runtime invisible and<br>
            visible annotations of method's parameters? Seems those that<br>
            appear later will overwrite those that appear sooner?<br>
<br>
<br>
        Oops, thanks. The way your patch handles that looks good to me.<br>
<br>
            -the MethodSymbol.savedParameterAnn<wbr>otations is only used<br>
            during reading inside the ClassReader, right? It seems<br>
            wasteful to have it as a field on each MethodSymbol, better<br>
            would be a field in ClassReader.<br>
<br>
<br>
        Sounds good. I'll try to avoid having savedParameterNames as a<br>
        field in MethodSymbol also. Do you remember if you encountered<br>
        any issues with that in your patch?<br>
<br>
            -please check what happens for annotations on constructors<br>
            of enums/non-static innerclasses<br>
<br>
<br>
        Will do. (Also, note that there appears to be an issue with<br>
        reading MethodParameters on constructors of enums/non-static<br>
        inner classes: JDK-8177486)<br>
<br>
<br>
<br>
</span></blockquote>
</blockquote></div><br></div>