Discussion:
RFR (M) 8211131: Remove the NSK_CPP_STUB macros from vmTestbase for jvmti/[G-I]*
JC Beyler
2018-09-27 03:37:51 UTC
Permalink
Hi all,

I continued the NSK_CPP_STUB removal with this webrev:

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

This does the first 50 files of the jvmti subfolder, it is done
automatically by the scripts I put in the bug comments.

Let me know what you think,
Jc
Alex Menkov
2018-10-02 00:55:57 UTC
Permalink
Hi Jc,

Looking at your conversion script I expected things like

if (!NSK_JVMTI_VERIFY(
NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {

to be converted to

if (!NSK_JVMTI_VERIFY(jvmti->AddCapabilities(&caps))) {

(then the final string is shorter than 100 symbols)

But actually I see

if (!NSK_JVMTI_VERIFY(
- NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
+ jvmti->AddCapabilities(&caps))) {

--alex
Post by JC Beyler
Hi all,
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211131/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8211131
This does the first 50 files of the jvmti subfolder, it is done
automatically by the scripts I put in the bug comments.
Let me know what you think,
Jc
JC Beyler
2018-10-02 13:51:08 UTC
Permalink
Hi Alex,

That is because this webrev was done before I added the 100 character wrap,
which I added when I was generating the next batch of changes. Here is the
webrev with the new version of the script:

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

Thanks, let me know what you think!
Jc
Post by Alex Menkov
Hi Jc,
Looking at your conversion script I expected things like
if (!NSK_JVMTI_VERIFY(
NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
to be converted to
if (!NSK_JVMTI_VERIFY(jvmti->AddCapabilities(&caps))) {
(then the final string is shorter than 100 symbols)
But actually I see
if (!NSK_JVMTI_VERIFY(
- NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
+ jvmti->AddCapabilities(&caps))) {
--alex
Post by JC Beyler
Hi all,
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211131/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8211131
This does the first 50 files of the jvmti subfolder, it is done
automatically by the scripts I put in the bug comments.
Let me know what you think,
Jc
--
Thanks,
Jc
Alex Menkov
2018-10-02 21:03:46 UTC
Permalink
Hi Jc,

This looks much better to me :)
Sometimes the script generate too long lines like
- if (!NSK_JNI_VERIFY(jni, (testedMeth =
NSK_CPP_STUB4(GetStaticMethodID,
- jni, objCls, meth_sig[clsIdx][i][0],
- meth_sig[clsIdx][i][2])) != NULL)) {
+ if (!NSK_JNI_VERIFY(jni, (testedMeth =
jni->GetStaticMethodID(objCls, meth_sig[clsIdx][i][0],
meth_sig[clsIdx][i][2])) != NULL)) {

But I think it would be better to fix them clearing NSK_JNI_VERIFY stuff.

So LGTM.

--alex
Post by JC Beyler
Hi Alex,
That is because this webrev was done before I added the 100 character
wrap, which I added when I was generating the next batch of changes.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211131/webrev.01
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.01>
Bug: https://bugs.openjdk.java.net/browse/JDK-8211131
Thanks, let me know what you think!
Jc
Hi Jc,
Looking at your conversion script I expected things like
          if (!NSK_JVMTI_VERIFY(
                NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
to be converted to
if (!NSK_JVMTI_VERIFY(jvmti->AddCapabilities(&caps))) {
(then the final string is shorter than 100 symbols)
But actually I see
if (!NSK_JVMTI_VERIFY(
-                NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
+                jvmti->AddCapabilities(&caps))) {
--alex
Post by JC Beyler
Hi all,
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211131/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.00/>
Post by JC Beyler
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8211131
This does the first 50 files of the jvmti subfolder, it is done
automatically by the scripts I put in the bug comments.
Let me know what you think,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-10-03 05:22:39 UTC
Permalink
Hi Jc,

Looks good to me.

Thanks,
Serguei
Post by JC Beyler
Hi Alex,
That is because this webrev was done before I added the 100 character
wrap, which I added when I was generating the next batch of changes.
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211131/webrev.01
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.01>
Bug: https://bugs.openjdk.java.net/browse/JDK-8211131
Thanks, let me know what you think!
Jc
Hi Jc,
Looking at your conversion script I expected things like
          if (!NSK_JVMTI_VERIFY(
                NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
to be converted to
if (!NSK_JVMTI_VERIFY(jvmti->AddCapabilities(&caps))) {
(then the final string is shorter than 100 symbols)
But actually I see
if (!NSK_JVMTI_VERIFY(
-                NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
+                jvmti->AddCapabilities(&caps))) {
--alex
Post by JC Beyler
Hi all,
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211131/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.00/>
Post by JC Beyler
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8211131
This does the first 50 files of the jvmti subfolder, it is done
automatically by the scripts I put in the bug comments.
Let me know what you think,
Jc
--
Thanks,
Jc
JC Beyler
2018-10-05 18:04:33 UTC
Permalink
Hi Alex and Serguei,

Thanks for the reviews! I will push it through the submit and then push
then!
Jc

Ps: the really long lines, as you said will get fixed in subsequent CLs so
it should disappear in the very near future
Post by Alex Menkov
Hi Jc,
Looks good to me.
Thanks,
Serguei
Hi Alex,
That is because this webrev was done before I added the 100 character
wrap, which I added when I was generating the next batch of changes. Here
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211131/webrev.01
Bug: https://bugs.openjdk.java.net/browse/JDK-8211131
Thanks, let me know what you think!
Jc
Post by Alex Menkov
Hi Jc,
Looking at your conversion script I expected things like
if (!NSK_JVMTI_VERIFY(
NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
to be converted to
if (!NSK_JVMTI_VERIFY(jvmti->AddCapabilities(&caps))) {
(then the final string is shorter than 100 symbols)
But actually I see
if (!NSK_JVMTI_VERIFY(
- NSK_CPP_STUB2(AddCapabilities, jvmti, &caps))) {
+ jvmti->AddCapabilities(&caps))) {
--alex
Post by JC Beyler
Hi all,
Webrev: http://cr.openjdk.java.net/~jcbeyler/8211131/webrev.00/
<http://cr.openjdk.java.net/%7Ejcbeyler/8211131/webrev.00/>
Bug: https://bugs.openjdk.java.net/browse/JDK-8211131
This does the first 50 files of the jvmti subfolder, it is done
automatically by the scripts I put in the bug comments.
Let me know what you think,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
Loading...