<div dir="ltr">Thanks for the reminder, done:<div><a href="http://cr.openjdk.java.net/~cushon/8190452/webrev.03/" class="cremed">http://cr.openjdk.java.net/~cushon/8190452/webrev.03/</a><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Feb 3, 2018 at 8:59 PM, Jonathan Gibbons <span dir="ltr"><<a href="mailto:jonathan.gibbons@oracle.com" target="_blank">jonathan.gibbons@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div text="#000000" bgcolor="#FFFFFF">
    <p>Liam,</p>
    <p>From a minor stylistic point of view, we generally place the test
      description (the comment beginning @test) after the legal header,
      and before any code like package and import statements<br>
    </p>
    -- Jon<div><div class="h5"><br>
    <br>
    <div class="m_-3189702744487010641moz-cite-prefix">On 2/3/18 6:27 PM, Liam Miller-Cushon
      wrote:<br>
    </div>
    <blockquote type="cite">
      <div dir="ltr">I reworked the test to use class reading, and added
        the -Xlint:option warning.
        <div><br>
          <div>Here's the updated patch: <a href="http://cr.openjdk.java.net/%7Ecushon/8190452/webrev.02/" class="m_-3189702744487010641cremed" target="_blank">http://cr.openjdk.java.<wbr>net/~cushon/8190452/webrev.02/</a></div>
        </div>
      </div>
      <div class="gmail_extra"><br>
        <div class="gmail_quote">On Sat, Feb 3, 2018 at 10:39 AM,
          Vicente Romero <span dir="ltr"><<a href="mailto:vicente.romero@oracle.com" target="_blank">vicente.romero@oracle.com</a>></span>
          wrote:<br>
          <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
            <div text="#000000" bgcolor="#FFFFFF"><span> <br>
                <br>
                <div class="m_-3189702744487010641m_8434774505278310383moz-cite-prefix">On
                  02/03/2018 12:37 AM, Liam Miller-Cushon wrote:<br>
                </div>
              </span><span>
                <blockquote type="cite">
                  <div dir="ltr">Hi <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Vicente,
                      thanks for the review!</span>
                    <div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br>
                      </span></div>
                    <div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">I
                        inlined the golden files into the test class. I
                        don't think inlining Lib.java works with the
                        current approach of using `@compile --release
                        7/8` and reflection to access the parameter
                        names, because the reflective APIs were added in
                        8. I could rewrite the test to use class reading
                        instead of reflection if you prefer.</span></div>
                  </div>
                </blockquote>
                <br>
              </span> yes I was thinking about class reading but I'm OK
              with the current version of the patch<span><br>
                <br>
                <blockquote type="cite">
                  <div dir="ltr">
                    <div>
                      <div><br>
                      </div>
                      <div>Here's the updated webrev: <span style="font-size:12.8px"><a href="http://cr.openjdk.java.net/%7Ecushon/8190452/webrev.01/" class="m_-3189702744487010641m_8434774505278310383cremed" target="_blank">http://cr.openjdk.java<wbr>.net/~cushon/8190452/webrev.<wbr>01/</a></span></div>
                    </div>
                    <div><br>
                    </div>
                    <div>> <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">Regarding
                        your questions about what to do with the
                        -Xlint:X options. I don't have any opinion on
                        one way or the other, is there any reason to
                        change them?</span></div>
                    <div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br>
                      </span></div>
                    <div><span style="font-size:12.8px">I thought it was
                        potentially surprising that -parameters will now
                        be silently ignored for Java 7 and earlier
                        language levels. Warning about that flag
                        combination would make the new behaviour more
                        discoverable.</span><br>
                    </div>
                    <div><span style="font-size:12.8px"><br>
                      </span></div>
                    <div><span style="font-size:12.8px">Since the bug
                        shipped in 9 and 10 there are some <v52 class
                        files with MethodParameters in the wild, and
                        I've seen cases where it broke builds using
                        -Xlint:classfile and -Werror.</span></div>
                    <div><span style="font-size:12.8px"><br>
                      </span></div>
                    <div><span style="font-size:12.8px"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">I
                          don't think either of those are common
                          problems. If you think we should leave the
                          -Xlint handling as-is that sounds good to me.</span><br>
                      </span></div>
                  </div>
                </blockquote>
                <br>
              </span> as discussed off-line with Jon, yes please add the
              warning, but feel free to do it as part of this bug or in
              a different one.<span class="m_-3189702744487010641HOEnZb"><font color="#888888"><br>
                  <br>
                  Vicente</font></span><span><br>
                <br>
                <blockquote type="cite">
                  <div class="gmail_extra"><br>
                    <div class="gmail_quote">On Fri, Feb 2, 2018 at 6:17
                      AM, Vicente Romero <span dir="ltr"><<a href="mailto:vicente.romero@oracle.com" target="_blank">vicente.romero@oracle.com</a>></span>
                      wrote:<br>
                      <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                        <div text="#000000" bgcolor="#FFFFFF"> Hi Liam,<br>
                          <br>
                          The fix looks OK, regarding the test, I don't
                          see the need for the two golden files as they
                          can be constants in the test per se. In
                          addition, the whole test could be self
                          contained in only one class that compiles the
                          Lib.java source. Regarding your questions
                          about what to do with the -Xlint:X options. I
                          don't have any opinion on one way or the
                          other, is there any reason to change them?<br>
                          <br>
                          Thanks,<br>
                          Vicente
                          <div>
                            <div class="m_-3189702744487010641m_8434774505278310383h5"><br>
                              <br>
                              <div class="m_-3189702744487010641m_8434774505278310383m_-3429153412801308596moz-cite-prefix">On
                                02/01/2018 01:56 PM, Liam Miller-Cushon
                                wrote:<br>
                              </div>
                              <blockquote type="cite">
                                <div dir="ltr">Bump. I'm happy to
                                  implement either of the alternatives I
                                  mentioned, but was hoping to get
                                  feedback on the approach first.</div>
                                <div class="gmail_extra"><br>
                                  <div class="gmail_quote">On Wed, Jan
                                    17, 2018 at 11:51 AM, Liam
                                    Miller-Cushon <span dir="ltr"><<a href="mailto:cushon@google.com" target="_blank">cushon@google.com</a>></span>
                                    wrote:<br>
                                    <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                                      <div dir="ltr">
                                        <div>Please review a fix for <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">JDK-8190452.
                                            The change causes javac to
                                            not emit </span>MethodParameters
                                          attributes when targeting v51
                                          class files.</div>
                                        <div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br>
                                          </span></div>
                                        <div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">The
                                            change implements the
                                            suggestion from this thread:</span><br>
                                        </div>
                                        <div><a href="http://mail.openjdk.java.net/pipermail/compiler-dev/2018-January/011579.html" target="_blank">http://mail.openjdk.java.net/p<wbr>ipermail/compiler-dev/2018-Jan<wbr>uary/011579.html</a><br>
                                        </div>
                                        <div><br>
                                        </div>
                                        <div>There are two related
                                          changes that may be worth
                                          considering:</div>
                                        <div>* now that -parameters will
                                          be ignored when compiling with
                                          --release < 8, should this
                                          combination of flags result in
                                          a warning if -Xlint:options is
                                          enabled?</div>
                                        <div>* since this wasn't fixed
                                          in JDK 9, there are v51 class
                                          files in the wild that contain
                                          unexpected MethodParameters
                                          attributes. Should
                                          -Xlint:classfile be relaxed to
                                          avoid warning on those?</div>
                                        <div><br>
                                        </div>
                                        <div>bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8190452" target="_blank">https://bugs.openjdk.java<wbr>.net/browse/JDK-8190452</a><br>
                                        </div>
                                        <div>webrev: <a href="http://cr.openjdk.java.net/%7Ecushon/8190452/webrev.00/" target="_blank">http://cr.openjdk.java<wbr>.net/~cushon/8190452/webrev.00<wbr>/</a></div>
                                      </div>
                                    </blockquote>
                                  </div>
                                  <br>
                                </div>
                              </blockquote>
                              <br>
                            </div>
                          </div>
                        </div>
                      </blockquote>
                    </div>
                    <br>
                  </div>
                </blockquote>
                <br>
              </span></div>
          </blockquote>
        </div>
        <br>
      </div>
    </blockquote>
    <br>
  </div></div></div>

</blockquote></div><br></div>