[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (M) 8210182: Remove macros for C compilation from vmTestBase but non jvmti
From: Chris Plummer <chris.plummer () oracle ! com>
Date: 2018-08-31 23:02:27
Message-ID: 9adcb1cb-824d-c9a7-238b-af59e9f0e27b () oracle ! com
[Download RAW message or body]
<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>
Other than what Serguei pointed out, it looks good.<br>
<br>
thanks,<br>
<br>
Chris<br>
<br>
On 8/30/18 11:31 PM, <a class="moz-txt-link-abbreviated" \
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br> \
</div> <blockquote type="cite"
cite="mid:270cd0af-fb4f-e2f2-472e-c449ba4d3ba5@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<div class="moz-cite-prefix">Hi Jc,<br>
<br>
The fix looks great as it looks much more simple now!<br>
<br>
One minor suggestion about this file update:<br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210182/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/share/jpda/libNativeMethodsTestThread.cpp.udiff.html"
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210182/webrev.02/test \
/hotspot/jtreg/vmTestbase/nsk/share/jpda/libNativeMethodsTestThread.cpp.udiff.html</a><br>
<br>
An example:<br>
<pre><span class="new"> + env->CallVoidMethod(thisObject, \
env->GetMethodID(klass, "log", "(Ljava/lang/String;)V"),</span> <span class="new"> \
+ message);</span></pre> I'd suggest either to place one \
call at one line, or place each argument on a different line like this:<br>
<pre><span class="new"> + env->CallVoidMethod(thisObject,
+ env->GetMethodID(klass, "log", \
"(Ljava/lang/String;)V"),</span> <span class="new"> + \
message);
There are several similar cases in this file.
No need in another webrev if you fix this.
</span></pre>
Thanks,<br>
Serguei<br>
<br>
<br>
On 8/30/18 12:03, JC Beyler wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAF9BGBx=D0AKO+bat0Tv_pLiEMRR-i7eR7PA2=n6scfxc+zdSg@mail.gmail.com">
<div dir="ltr">Hi Alex,
<div><br>
</div>
<div>I've fixed those cases and found an extra one. I'll
revise my scripts for the next folder so these no longer
happen (bad regexp that eagerly finds a ')'.</div>
<div><br>
</div>
<div>I had launched in parallel a test for vmTestBase, it
finished with a few failures that might be linked to this so
I have relaunched the whole vmTestBase and will post the
results here.</div>
<div><br>
</div>
<div>The new webrev with the fixes is here:</div>
<div><a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210182/webrev.02/"
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8210182/webrev.02/</a><br>
</div>
<div><br>
</div>
<div>Thanks again for the review,</div>
<div>Jc</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Thu, Aug 30, 2018 at 10:48 AM Alex Menkov
<<a href="mailto:alexey.menkov@oracle.com"
moz-do-not-send="true">alexey.menkov@oracle.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote">Hi Jc,<br>
<br>
It looks much better now.<br>
BTW did you run all the tests?<br>
<br>
test/hotspot/jtreg/vmTestbase/nsk/share/jni/JNIreferences.cpp<br>
"(V" should be "()V":<br>
<br>
// notify another thread that JNI local reference
has been created<br>
-
JNI_ENV_PTR(env)->CallVoidMethod(JNI_ENV_ARG_3(env, <br>
createWicket,
JNI_ENV_PTR(env)->GetMethodID(JNI_ENV_ARG_4(env, klass, <br>
"unlock", "()V"))));<br>
+ env->CallVoidMethod(createWicket,
env->GetMethodID(klass, <br>
"unlock", "(V"));<br>
<br>
// wait till JNI local reference can be released
(it will <br>
heppen then we will leave the method)<br>
-
JNI_ENV_PTR(env)->CallVoidMethod(JNI_ENV_ARG_3(env, <br>
deleteWicket,
JNI_ENV_PTR(env)->GetMethodID(JNI_ENV_ARG_4(env, klass, <br>
"waitFor", "()V"))));<br>
+ env->CallVoidMethod(deleteWicket,
env->GetMethodID(klass, <br>
"waitFor", "(V"));<br>
}<br>
<br>
--alex<br>
<br>
On 08/29/2018 22:01, JC Beyler wrote:<br>
> Hi all,<br>
> <br>
> A follow-up to Igor's work on getting tests in C++, I
am working on <br>
> simplifying the macros in the tests from the
vmTestBase. The full change <br>
> being a bit too large, I'm cutting it up in pieces to
be easier to <br>
> review and integrate.<br>
> <br>
> Here is the first part, it changes all vmTestbase tests
outside the <br>
> vmTestbase/jvmti subfolder:<br>
> <br>
> Webrev: <a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210182/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~jcbeyler/8210182/webrev.01/</a> \
<br> > <<a
href="http://cr.openjdk.java.net/%7Ejcbeyler/8210182/webrev.01/"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/%7Ejcbeyler/8210182/webrev.01/</a>><br>
> Bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8210182"
rel="noreferrer" target="_blank" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8210182</a><br> > \
<br> > Thanks!<br>
> Jc<br>
</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>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic