Re: RFR (L) 8210481: Remove #ifdef cplusplus from vmTestbase

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR (L) 8210481: Remove #ifdef cplusplus from vmTestbase

Igor Ignatyev
Hi JC,

looks good to me.

(cc'ing hotspot-dev alias as it affects other teams' tests)

-- Igor

> On Sep 14, 2018, at 9:50 AM, JC Beyler <[hidden email]> wrote:
>
> Hi all,
>
> Could I get a review for the following webrev:
> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210481/webrev.00/ <http://cr.openjdk.java.net/~jcbeyler/8210481/webrev.00/>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210481 <https://bugs.openjdk.java.net/browse/JDK-8210481>
>
> @Alex: I know we had said generally 50 files max but this one is really straight forward; if you still prefer 50 files, let me know and I'll cut it up into chunks.
>
> @all: A big webrev this time, to which I apologize but it is the last macro removal webrev from me in a while (there still is https://bugs.openjdk.java.net/browse/JDK-8210728 <https://bugs.openjdk.java.net/browse/JDK-8210728> but I'm giving everyone a rest on macro removal and will work on a few code refactoring I promised to do ;-)).
>
> To perhaps help reviewing it:
> This webrev removes two #ifdef per file touched:
>
> -#ifdef __cplusplus
>  extern "C" {
> -#endif
>
> and
>
> -#ifdef __cplusplus
>  }
> -#endif
>
> -> The patch only has deletions (4 per file), no insertions
> -> I double checked that all files only have exactly those removals and no other removals EXCEPT:
>  http://cr.openjdk.java.net/~jcbeyler/8210481/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h.udiff.html <http://cr.openjdk.java.net/~jcbeyler/8210481/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h.udiff.html>
> which is still defining the NSK_STUB macros but was doing it for C/C++ styles.
> -> Last easier check for reviewers:
>    - Look for deletions but that are not exactly "-#endif" or "-#ifdef __cplusplus"
> grep "^-[^-]" jdk12-jvmti12.patch | grep -v "^-#endif$" | grep -v "^-#ifdef __cplusplus$"
>
> That command provides only the lines from the nsk_tools.h file:
> $ grep "^-[^-]" jdk12-jvmti12.patch | grep -v "^-#endif$" | grep -v "^-#ifdef __cplusplus$"
> -#define NSK_CPP_STUB1(Func,env)  (*env)->Func(env)
> -#define NSK_CPP_STUB2(Func,env,a)  (*env)->Func(env,a)
> -#define NSK_CPP_STUB3(Func,env,a,b)  (*env)->Func(env,a,b)
> -#define NSK_CPP_STUB4(Func,env,a,b,c)  (*env)->Func(env,a,b,c)
> -#define NSK_CPP_STUB5(Func,env,a,b,c,d)  (*env)->Func(env,a,b,c,d)
> -#define NSK_CPP_STUB6(Func,env,a,b,c,d,e)  (*env)->Func(env,a,b,c,d,e)
> -#define NSK_CPP_STUB7(Func,env,a,b,c,d,e,f)  (*env)->Func(env,a,b,c,d,e,f)
> -#define NSK_CPP_STUB8(Func,env,a,b,c,d,e,f,g)  (*env)->Func(env,a,b,c,d,e,f,g)
> -#define NSK_CPP_STUB9(Func,env,a,b,c,d,e,f,g,h)  (*env)->Func(env,a,b,c,d,e,f,g,h)
> -#ifndef NSK_CPP_STUBS_ENFORCE_C
> -#undef NSK_CPP_STUB1
> -#undef NSK_CPP_STUB2
> -#undef NSK_CPP_STUB3
> -#undef NSK_CPP_STUB4
> -#undef NSK_CPP_STUB5
> -#undef NSK_CPP_STUB6
> -#undef NSK_CPP_STUB7
> -#undef NSK_CPP_STUB8
> -#undef NSK_CPP_STUB9
>
> Thanks for the reviews,
> Jc

Reply | Threaded
Open this post in threaded view
|

Re: RFR (L) 8210481: Remove #ifdef cplusplus from vmTestbase

David Holmes
+1 from me.

Thanks,
David

On 15/09/2018 3:48 AM, Igor Ignatyev wrote:

> Hi JC,
>
> looks good to me.
>
> (cc'ing hotspot-dev alias as it affects other teams' tests)
>
> -- Igor
>
>> On Sep 14, 2018, at 9:50 AM, JC Beyler <[hidden email]> wrote:
>>
>> Hi all,
>>
>> Could I get a review for the following webrev:
>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8210481/webrev.00/ <http://cr.openjdk.java.net/~jcbeyler/8210481/webrev.00/>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210481 <https://bugs.openjdk.java.net/browse/JDK-8210481>
>>
>> @Alex: I know we had said generally 50 files max but this one is really straight forward; if you still prefer 50 files, let me know and I'll cut it up into chunks.
>>
>> @all: A big webrev this time, to which I apologize but it is the last macro removal webrev from me in a while (there still is https://bugs.openjdk.java.net/browse/JDK-8210728 <https://bugs.openjdk.java.net/browse/JDK-8210728> but I'm giving everyone a rest on macro removal and will work on a few code refactoring I promised to do ;-)).
>>
>> To perhaps help reviewing it:
>> This webrev removes two #ifdef per file touched:
>>
>> -#ifdef __cplusplus
>>   extern "C" {
>> -#endif
>>
>> and
>>
>> -#ifdef __cplusplus
>>   }
>> -#endif
>>
>> -> The patch only has deletions (4 per file), no insertions
>> -> I double checked that all files only have exactly those removals and no other removals EXCEPT:
>>   http://cr.openjdk.java.net/~jcbeyler/8210481/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h.udiff.html <http://cr.openjdk.java.net/~jcbeyler/8210481/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h.udiff.html>
>> which is still defining the NSK_STUB macros but was doing it for C/C++ styles.
>> -> Last easier check for reviewers:
>>     - Look for deletions but that are not exactly "-#endif" or "-#ifdef __cplusplus"
>> grep "^-[^-]" jdk12-jvmti12.patch | grep -v "^-#endif$" | grep -v "^-#ifdef __cplusplus$"
>>
>> That command provides only the lines from the nsk_tools.h file:
>> $ grep "^-[^-]" jdk12-jvmti12.patch | grep -v "^-#endif$" | grep -v "^-#ifdef __cplusplus$"
>> -#define NSK_CPP_STUB1(Func,env)  (*env)->Func(env)
>> -#define NSK_CPP_STUB2(Func,env,a)  (*env)->Func(env,a)
>> -#define NSK_CPP_STUB3(Func,env,a,b)  (*env)->Func(env,a,b)
>> -#define NSK_CPP_STUB4(Func,env,a,b,c)  (*env)->Func(env,a,b,c)
>> -#define NSK_CPP_STUB5(Func,env,a,b,c,d)  (*env)->Func(env,a,b,c,d)
>> -#define NSK_CPP_STUB6(Func,env,a,b,c,d,e)  (*env)->Func(env,a,b,c,d,e)
>> -#define NSK_CPP_STUB7(Func,env,a,b,c,d,e,f)  (*env)->Func(env,a,b,c,d,e,f)
>> -#define NSK_CPP_STUB8(Func,env,a,b,c,d,e,f,g)  (*env)->Func(env,a,b,c,d,e,f,g)
>> -#define NSK_CPP_STUB9(Func,env,a,b,c,d,e,f,g,h)  (*env)->Func(env,a,b,c,d,e,f,g,h)
>> -#ifndef NSK_CPP_STUBS_ENFORCE_C
>> -#undef NSK_CPP_STUB1
>> -#undef NSK_CPP_STUB2
>> -#undef NSK_CPP_STUB3
>> -#undef NSK_CPP_STUB4
>> -#undef NSK_CPP_STUB5
>> -#undef NSK_CPP_STUB6
>> -#undef NSK_CPP_STUB7
>> -#undef NSK_CPP_STUB8
>> -#undef NSK_CPP_STUB9
>>
>> Thanks for the reviews,
>> Jc
>