Discussion:
RFR (M) 8212884: Remove the assignments in ifs for vmTestbase/[a-s]
(too old to reply)
JC Beyler
2018-10-24 16:40:40 UTC
Permalink
Hi all,

Here is the first webrev to extract assignments from if test clauses. It
should be relatively straightforward to review.

I tested all the tests changed and they pass on my dev machine.

Most changes were done via the script I put in the bug, I had to do a few
clean-ups and corner cases as well.

Webrev: http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8212884

Thanks,
Jc
Chris Plummer
2018-10-24 17:40:30 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi JC,<br>
<br>
Overall looks good. Just a couple of minor things. I don't need to
see another webrev.<br>
<br>
In Abort.cpp, Callbacks.cpp, ConcreteKlassFilter.cpp,
HeapFilter.cpp.frames, NonConcreteKlassFilter.cpp, and
threadend002.cpp.frames you need to add a space after the "if":<br>
<br>
 130   if(!NSK_VERIFY(jvmti != NULL)) {<br>
<br>
In em01t002.cpp, indent of 2nd line isn't correct:<br>
<br>
  74     methodID = jni_env-&gt;GetMethodID(<br>
  75                 klass, "loadClass",
"(Ljava/lang/String;)Ljava/lang/Class;");<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 10/24/18 9:40 AM, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBz4+Y-fHph41tthHb1Ub=m5-6-HJ6QXFV+***@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>Here is the first webrev to extract assignments from if
test clauses. It should be relatively straightforward to
review.</div>
<div><br>
</div>
<div>I tested all the tests changed and they pass on my dev
machine.<br clear="all">
<div><br>
</div>
<div>Most changes were done via the script I put in the
bug, I had to do a few clean-ups and corner cases as
well.</div>
<div><br>
</div>
<div dir="ltr" class="gmail_signature">
<div dir="ltr">
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212884"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212884</a></div>
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
</body>
</html>
JC Beyler
2018-10-24 20:00:24 UTC
Permalink
Hi Chris, thanks for the review!

I created https://bugs.openjdk.java.net/browse/JDK-8212939 for the
remaining spaces missing after an if/while/for/switch and between ){ in
the case of an if:
jint Agent_Initialize(JavaVM *vm, char *options, void *reserved){

Thanks,
Jc
Post by Chris Plummer
Hi JC,
Overall looks good. Just a couple of minor things. I don't need to see
another webrev.
In Abort.cpp, Callbacks.cpp, ConcreteKlassFilter.cpp,
HeapFilter.cpp.frames, NonConcreteKlassFilter.cpp, and
130 if(!NSK_VERIFY(jvmti != NULL)) {
74 methodID = jni_env->GetMethodID(
75 klass, "loadClass",
"(Ljava/lang/String;)Ljava/lang/Class;");
thanks,
Chris
Hi all,
Here is the first webrev to extract assignments from if test clauses. It
should be relatively straightforward to review.
I tested all the tests changed and they pass on my dev machine.
Most changes were done via the script I put in the bug, I had to do a few
clean-ups and corner cases as well.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8212884
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-10-24 21:26:30 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
It looks great!<br>
Your AI-enabled conversion scripts are smart! :)<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 10/24/18 09:40, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBz4+Y-fHph41tthHb1Ub=m5-6-HJ6QXFV+***@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>Here is the first webrev to extract assignments from if
test clauses. It should be relatively straightforward to
review.</div>
<div><br>
</div>
<div>I tested all the tests changed and they pass on my dev
machine.<br>
<div><br>
</div>
<div>Most changes were done via the script I put in the
bug, I had to do a few clean-ups and corner cases as
well.</div>
<div><br>
</div>
<div dir="ltr" class="gmail_signature">
<div dir="ltr">
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212884"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212884</a></div>
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>
JC Beyler
2018-10-25 16:00:29 UTC
Permalink
Hi all,

For reference, after the space removal webrev has been pushed, this webrev
had a tiny conflict (as Chris had guessed) in one file:
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp

You can see the old change vs the new change, it is trivial to see it is ok:
-
http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html
-
http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html

The full webrev is here:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8212884

If I could have a final LGTM, that would be awesome!
Jc
Post by s***@oracle.com
Hi Jc,
It looks great!
Your AI-enabled conversion scripts are smart! :)
Thanks,
Serguei
Hi all,
Here is the first webrev to extract assignments from if test clauses. It
should be relatively straightforward to review.
I tested all the tests changed and they pass on my dev machine.
Most changes were done via the script I put in the bug, I had to do a few
clean-ups and corner cases as well.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8212884
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-10-25 16:38:03 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
LGTM.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 10/25/18 09:00, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBx2eNFp4wOAsk2WcHh=Z5Bk+3LvVt95XCc1X+***@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>For reference, after the space removal webrev has
been pushed, this webrev had a tiny conflict (as
Chris had guessed) in one file:<br>
</div>
<div>test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp<br>
</div>
<div><br>
</div>
<div>You can see the old change vs the new change, it
is trivial to see it is ok:</div>
<div>- <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html</a><br>
</div>
<div>- <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html</a><br>
</div>
<div><br>
</div>
<div>The full webrev is here:</div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.01/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212884"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212884</a></div>
<div><br>
</div>
<div>If I could have a final LGTM, that would be
awesome!</div>
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Oct 24, 2018 at 2:26 PM <a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div class="m_7524423201872714834moz-cite-prefix">Hi Jc,<br>
<br>
It looks great!<br>
Your AI-enabled conversion scripts are smart! :)<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 10/24/18 09:40, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>Here is the first webrev to extract assignments
from if test clauses. It should be relatively
straightforward to review.</div>
<div><br>
</div>
<div>I tested all the tests changed and they pass on
my dev machine.<br>
<div><br>
</div>
<div>Most changes were done via the script I put
in the bug, I had to do a few clean-ups and
corner cases as well.</div>
<div><br>
</div>
<div dir="ltr"
class="m_7524423201872714834gmail_signature">
<div dir="ltr">
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.00/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212884"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212884</a></div>
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
<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>
<br>
</body>
</html>
Chris Plummer
2018-10-25 17:05:56 UTC
Permalink
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">+1<br>
<br>
On 10/25/18 9:38 AM, <a class="moz-txt-link-abbreviated" href="mailto:***@oracle.com">***@oracle.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:462ecc44-d226-1342-14f7-***@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
LGTM.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
On 10/25/18 09:00, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBx2eNFp4wOAsk2WcHh=Z5Bk+3LvVt95XCc1X+***@mail.gmail.com">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>For reference, after the space removal webrev
has been pushed, this webrev had a tiny conflict
(as Chris had guessed) in one file:<br>
</div>
<div>test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp<br>
</div>
<div><br>
</div>
<div>You can see the old change vs the new change,
it is trivial to see it is ok:</div>
<div>- <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html</a><br>
</div>
<div>- <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.udiff.html</a><br>
</div>
<div><br>
</div>
<div>The full webrev is here:</div>
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.01/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212884"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212884</a></div>
<div><br>
</div>
<div>If I could have a final LGTM, that would be
awesome!</div>
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, Oct 24, 2018 at 2:26 PM <a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a> &lt;<a
href="mailto:***@oracle.com"
moz-do-not-send="true">***@oracle.com</a>&gt;
wrote:<br>
</div>
<blockquote class="gmail_quote">
<div>
<div class="m_7524423201872714834moz-cite-prefix">Hi Jc,<br>
<br>
It looks great!<br>
Your AI-enabled conversion scripts are smart! :)<br>
<br>
Thanks,<br>
Serguei<br>
<br>
On 10/24/18 09:40, JC Beyler wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">Hi all,
<div><br>
</div>
<div>Here is the first webrev to extract
assignments from if test clauses. It should be
relatively straightforward to review.</div>
<div><br>
</div>
<div>I tested all the tests changed and they pass
on my dev machine.<br>
<div><br>
</div>
<div>Most changes were done via the script I put
in the bug, I had to do a few clean-ups and
corner cases as well.</div>
<div><br>
</div>
<div dir="ltr"
class="m_7524423201872714834gmail_signature">
<div dir="ltr">
<div>Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8212884/webrev.00/"
target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8212884/webrev.00/</a></div>
<div>Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8212884"
target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8212884</a></div>
<div><br>
</div>
Thanks,
<div>Jc</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
<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>
<br>
</blockquote>
<p><br>
</p>
</body>
</html>

Loading...