<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    looks good to me, only one minor comment, consider using a more
    meaningful name for:<br>
    <br>
    test/langtools/tools/javac/MethodParameters/8189708/Test.java<br>
    <br>
    there is no need for another review iteration for this change,<br>
    <br>
    Thanks,<br>
    Vicente<br>
    <br>
    <div class="moz-cite-prefix">On 12/13/2017 09:02 PM, Liam
      Miller-Cushon wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CAL4Qsgu==iRbTuZ8oHO1P7cBFnR7Pt2FDT99KRWR6fuXsGNccQ@mail.gmail.com">
      <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/%7Ecushon/8007720/webrev.02/"
            moz-do-not-send="true">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"
              moz-do-not-send="true">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"
                  moz-do-not-send="true">cushon@google.com</a><br>
              </span><span class="">
                <mailto:<a href="mailto:cushon@google.com"
                  target="_blank" moz-do-not-send="true">cushon@google.com</a>>>
                wrote:<br>
                <br>
                    Here's an updated patch that incorporates your
                approach:<br>
                    <a
                  href="http://cr.openjdk.java.net/%7Ecushon/8007720/webrev.01/"
                  rel="noreferrer" target="_blank"
                  moz-do-not-send="true">http://cr.openjdk.java.net/~cu<wbr>shon/8007720/webrev.01/</a><br>
                    <<a
                  href="http://cr.openjdk.java.net/%7Ecushon/8007720/webrev.01/"
                  rel="noreferrer" target="_blank"
                  moz-do-not-send="true">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" moz-do-not-send="true">cushon@google.com</a>
                <mailto:<a href="mailto:cushon@google.com"
                  target="_blank" moz-do-not-send="true">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" moz-do-not-send="true">jan.lahoda@oracle.com</a>
                <mailto:<a href="mailto:jan.lahoda@oracle.com"
                  target="_blank" moz-do-not-send="true">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>
    </blockquote>
    <br>
  </body>
</html>