<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">Thanks!<br>
      <br>
      On 10/5/18 4:02 PM, JC Beyler wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CAF9BGBxqH=8EgiPX23DXxt22L_6HLh3p7tGwf48YafRq2hcceA@mail.gmail.com">
      <meta http-equiv="content-type" content="text/html; charset=utf-8">
      <div dir="ltr">
        <div dir="ltr">Just for completeness: </div>
        <div dir="ltr"><br>
        </div>
        <div dir="ltr">@Chris, I created <a
            href="https://bugs.openjdk.java.net/browse/JDK-8211802"
            moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8211802</a>
          for the first case you were talking about, which is slightly
          different than assignments and perhaps we will do them
          together or not, we can see when I finish the macro
          replacements.</div>
        <div><br>
        </div>
        <div>Thanks!</div>
        <div>Jc</div>
      </div>
      <br>
      <div class="gmail_quote">
        <div dir="ltr">On Mon, Oct 1, 2018 at 11:59 AM Chris Plummer
          <<a href="mailto:chris.plummer@oracle.com"
            moz-do-not-send="true">chris.plummer@oracle.com</a>>
          wrote:<br>
        </div>
        <blockquote class="gmail_quote" style="margin:0 0 0
          .8ex;border-left:1px #ccc solid;padding-left:1ex">
          <div text="#000000" bgcolor="#FFFFFF">
            <div class="m_-1319000943628777899moz-cite-prefix">On
              10/1/18 9:50 AM, JC Beyler wrote:<br>
            </div>
            <blockquote type="cite">
              <div dir="ltr">
                <div dir="ltr">
                  <div dir="ltr">
                    <div dir="ltr">Hi Chris,
                      <div><br>
                      </div>
                      <div>Thanks for the review!</div>
                      <div><br>
                      </div>
                      <div>I think most of your comments are/should be
                        future items, I inlined my answers:</div>
                      <br>
                      <div class="gmail_quote">
                        <div dir="ltr">On Fri, Sep 28, 2018 at 7:49 PM
                          Chris Plummer <<a
                            href="mailto:chris.plummer@oracle.com"
                            target="_blank" moz-do-not-send="true">chris.plummer@oracle.com</a>>
                          wrote:<br>
                        </div>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">Hi JC,<br>
                          <br>
                          In addition to Alex's ForceEarlyReturn001.cpp
                          comment:<br>
                          <br>
                          There are many places where I see a space
                          between two parens at the end <br>
                          of the line. For example, in
                          attach020Agent00.cpp:<br>
                          <br>
                            168     if
                          (!NSK_JVMTI_VERIFY(jvmti->AddCapabilities(&caps))
                          ) {<br>
                          <br>
                        </blockquote>
                        <div><br>
                        </div>
                        <div>I saw that too, that will disappear when I
                          remove the NSK_JVMTI_VERIFY altogether. These
                          spaces were there before so I did not</div>
                        <div>want to try to fix them in this fix-up
                          round since anyway we were going to touch
                          these lines again anyway. </div>
                        <div><br>
                        </div>
                        <div>I looked and there are a lot of these
                          across the vmTestbase folders.</div>
                        <div><br>
                        </div>
                        <div>I created this bug to track it: <a
                            href="https://bugs.openjdk.java.net/browse/JDK-8211335"
                            target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8211335</a></div>
                        <div>(To be honest, every time I look a bit at
                          the lines fixed right now, I see things around
                          I'm itching to fix next but I strive to keep
                          the transformation simple).</div>
                        <div> </div>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex"> Every
                          place NSK_JNI_VERIFY is used there is ugliness
                          with "if" <br>
                          expressions involving JNI results that are not
                          already boolean. Besides <br>
                          a boolean being computed for the JNI result,
                          often there is also an <br>
                          assignment to the JNI result. I'm not sure if
                          you have plans to clean <br>
                          stuff like this up, but it would be best to
                          always do the assignment <br>
                          first, even if currently there is no local
                          variable being assigned to. <br>
                          It would simplify the boolean expression being
                          passed to NSK_JNI_VERIFY. <br>
                          Here's one example:<br>
                          <br>
                            137     if (!NSK_JNI_VERIFY(jni, (array =
                          (jbyteArray)<br>
                            138            
                          jni->GetStaticObjectField(cls, fieldID)) !=
                          NULL)) {<br>
                          <br>
                          This would be much better:<br>
                          <br>
                            137     array =
                          (jbyteArray)jni->GetStaticObjectField(cls,
                          fieldID);<br>
                            138     if (!NSK_JNI_VERIFY(jni, array !=
                          NULL) {<br>
                          <br>
                        </blockquote>
                        <div><br>
                        </div>
                        <div>I already have this on my future plate:</div>
                        <div><a
                            href="https://bugs.openjdk.java.net/browse/JDK-8210922"
                            target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8210922</a><br>
                        </div>
                      </div>
                    </div>
                  </div>
                </div>
              </div>
            </blockquote>
            Ok, but sometimes it's just a comparison of the result, but
            no assignment. You end up with the method call and args
            starting on one line, and the last args and closing paren on
            the next, followed by a compare op. Would be nice to instead
            create a local to assign the result to.<br>
            <blockquote type="cite">
              <div dir="ltr">
                <div dir="ltr">
                  <div dir="ltr">
                    <div dir="ltr">
                      <div class="gmail_quote">
                        <div><br>
                        </div>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          attach015Target.cpp: use of ?: is not needed
                          and it should be explicitly <br>
                          checking if FindClass is NULL.<br>
                          <br>
                             40     return
                          jni->FindClass(LOADED_CLASS_NAME) ?
                          JNI_TRUE : JNI_FALSE;<br>
                          <br>
                        </blockquote>
                        <div><br>
                        </div>
                        <div>For this, I saw a conversation in hotspot (<a
href="http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/033828.html"
                            target="_blank" moz-do-not-send="true">http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/033828.html</a>)
                          saying we have to be careful here. I think
                          returning != 0 will work, right? If so, I'll
                          create a bug to fix all of these up.</div>
                      </div>
                    </div>
                  </div>
                </div>
              </div>
            </blockquote>
            Yes, that is what i was suggesting, except compare to NULL,
            no 0.<br>
            <blockquote type="cite">
              <div dir="ltr">
                <div dir="ltr">
                  <div dir="ltr">
                    <div dir="ltr">
                      <div class="gmail_quote">
                        <div> <br>
                        </div>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex">
                          attach022Agent00.cpp: The empty line 88 and
                          141 should be removed. Also <br>
                          extra space near the end of the line:<br>
                        </blockquote>
                        <div><br>
                        </div>
                        <div>Done for the lines and the white space at
                          the end of the line.</div>
                      </div>
                    </div>
                  </div>
                </div>
              </div>
            </blockquote>
            ok.<br>
            <blockquote type="cite">
              <div dir="ltr">
                <div dir="ltr">
                  <div dir="ltr">
                    <div dir="ltr">
                      <div class="gmail_quote">
                        <div><br>
                        </div>
                        <div><br>
                        </div>
                        <div>
                          <div>Basically, my ordering of refactoring
                            would be if the reviewers agree:</div>
                          <div>   - Remove the NSK_CPP_STUB*  (which is
                            what these refactoring to do)</div>
                          <div>   - Remove the NSK_*_VERIFY macros at
                            which point I'll remove that space you saw
                            for NSK_*_VERIFY lines; that will remove the
                            ) in the lines</div>
                          <div>   - Move out the assignments</div>
                             - Remove the remaining spaces before a )
                          for l</div>
                        <div> </div>
                        <div>Let me know what you think,</div>
                      </div>
                    </div>
                  </div>
                </div>
              </div>
            </blockquote>
            <br>
            Looks good.<br>
            <br>
            Chris<br>
            <blockquote type="cite">
              <div dir="ltr">
                <div dir="ltr">
                  <div dir="ltr">
                    <div dir="ltr">
                      <div class="gmail_quote">
                        <div>Jc</div>
                        <div><br>
                        </div>
                        <blockquote class="gmail_quote"
                          style="margin:0px 0px 0px
                          0.8ex;border-left:1px solid
                          rgb(204,204,204);padding-left:1ex"> <br>
                          thanks,<br>
                          <br>
                          Chris<br>
                          <br>
                          On 9/27/18 10:15 PM, JC Beyler wrote:<br>
                          > Hi all,<br>
                          ><br>
                          > I continued the NSK_CPP_STUB removal with
                          this webrev:<br>
                          ><br>
                          > Webrev: <a
                            href="http://cr.openjdk.java.net/%7Ejcbeyler/8211261/webrev.00/"
                            rel="noreferrer" target="_blank"
                            moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8211261/webrev.00/</a>
                          <br>
                          > <<a
                            href="http://cr.openjdk.java.net/%7Ejcbeyler/8211261/webrev.00/"
                            rel="noreferrer" target="_blank"
                            moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8211261/webrev.00/</a>><br>
                          > Bug: <a
                            href="https://bugs.openjdk.java.net/browse/JDK-8211261"
                            rel="noreferrer" target="_blank"
                            moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8211261</a><br>
                          ><br>
                          > This does the another set of 50 files of
                          the jvmti subfolder, it is <br>
                          > done automatically by the scripts I put
                          in the bug comments.<br>
                          ><br>
                          > This passes the tests changed on my dev
                          machine.<br>
                          ><br>
                          > Let me know what you think,<br>
                          > Jc<br>
                          <br>
                          <br>
                          <br>
                        </blockquote>
                      </div>
                      <br clear="all">
                      <div><br>
                      </div>
                      -- <br>
                      <div dir="ltr"
class="m_-1319000943628777899gmail-m_3622677733063014630m_3563255169388879489gmail-m_-2163559460033312510m_-1086457419127703162gmail-m_-1699793861447603098m_6813899786495674574m_556791172373509929gmail_signature">
                        <div dir="ltr">
                          <div><br>
                          </div>
                          Thanks,
                          <div>Jc</div>
                        </div>
                      </div>
                    </div>
                  </div>
                </div>
              </div>
            </blockquote>
            <p><br>
            </p>
          </div>
        </blockquote>
      </div>
      <br clear="all">
      <div><br>
      </div>
      -- <br>
      <div dir="ltr" class="gmail_signature"
        data-smartmail="gmail_signature">
        <div dir="ltr">
          <div><br>
          </div>
          Thanks,
          <div>Jc</div>
        </div>
      </div>
    </blockquote>
    <p><br>
    </p>
  </body>
</html>