Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
> On 10 Jan 2014, at 19:40, Dan Xu wrote: > > Thank you for all the feedback. I have updated my changes to use > "CHECK_EXCEPTION_RETURN" and "CHECK_EXCEPTION" macro recently added into > jni_util.h. I also removed else block in function setStaticIntField() in > Version.c since (*env)->GetStaticFieldID will throw a same exception if the > field cannot be found. > > Here is the new webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.01/. > Thanks! Looks good to me Dan. -Chris. > > -Dan > > >> On 01/10/2014 10:21 AM, Mike Duigou wrote: >>> On Jan 10 2014, at 10:09 , Chris Hegarty wrote: >>> On 10 Jan 2014, at 18:05, Dan Xu wrote: Hi Roger, My macro is a little different from yours, which compares with -1 instead of NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are useful when I cannot decide the pending exception state by just using return values. As for the style, actually I prefer the (!pointer) to (pointer == NULL) because it is more concise and also make me avoid the typo like (pointer = NULL), which I cannot find from the compilation. Thanks! >> There's always "yoda conditions" >> https://en.wikipedia.org/wiki/Yoda_conditions, (NULL == pointer), but that's >> not likely to make anyone (besides me) happy. >> >> Mike >> >>> Not that it matters, but my preference is to == NULL. >>> >>> -Chris. >>> -Dan On 01/10/2014 08:40 AM, roger riggs wrote: > Hi Dan, > > Just pushed are macros in jni_util.h to do the same function as your new > macros. > Please update to use the common macros instead of introducing new ones. > > Style wise, I would avoid mixing binary operators (!) with pointers. > it is clearer to compare with NULL. (The CHECK_NULL macro will do the > check and return). > > (Not a Reviewer) > > Thanks, Roger > > > > On 1/10/2014 1:31 AM, Dan Xu wrote: >> Hi All, >> >> Please review the fix for JNI pending exception issues reported in >> jdk-8029007. Thanks! >> >> Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ >> Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 >> >> -Dan >
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Thank you for all the feedback. I have updated my changes to use "CHECK_EXCEPTION_RETURN" and "CHECK_EXCEPTION" macro recently added into jni_util.h. I also removed else block in function setStaticIntField() in Version.c since (*env)->GetStaticFieldID will throw a same exception if the field cannot be found. Here is the new webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.01/. Thanks! -Dan On 01/10/2014 10:21 AM, Mike Duigou wrote: On Jan 10 2014, at 10:09 , Chris Hegarty wrote: On 10 Jan 2014, at 18:05, Dan Xu wrote: Hi Roger, My macro is a little different from yours, which compares with -1 instead of NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are useful when I cannot decide the pending exception state by just using return values. As for the style, actually I prefer the (!pointer) to (pointer == NULL) because it is more concise and also make me avoid the typo like (pointer = NULL), which I cannot find from the compilation. Thanks! There's always "yoda conditions" https://en.wikipedia.org/wiki/Yoda_conditions, (NULL == pointer), but that's not likely to make anyone (besides me) happy. Mike Not that it matters, but my preference is to == NULL. -Chris. -Dan On 01/10/2014 08:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
On Jan 10 2014, at 10:09 , Chris Hegarty wrote: > On 10 Jan 2014, at 18:05, Dan Xu wrote: > >> Hi Roger, >> >> My macro is a little different from yours, which compares with -1 instead of >> NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are >> useful when I cannot decide the pending exception state by just using return >> values. >> >> As for the style, actually I prefer the (!pointer) to (pointer == NULL) >> because it is more concise and also make me avoid the typo like (pointer = >> NULL), which I cannot find from the compilation. Thanks! There's always "yoda conditions" https://en.wikipedia.org/wiki/Yoda_conditions, (NULL == pointer), but that's not likely to make anyone (besides me) happy. Mike > > Not that it matters, but my preference is to == NULL. > > -Chris. > >> >> -Dan >> >> >> On 01/10/2014 08:40 AM, roger riggs wrote: >>> Hi Dan, >>> >>> Just pushed are macros in jni_util.h to do the same function as your new >>> macros. >>> Please update to use the common macros instead of introducing new ones. >>> >>> Style wise, I would avoid mixing binary operators (!) with pointers. >>> it is clearer to compare with NULL. (The CHECK_NULL macro will do the >>> check and return). >>> >>> (Not a Reviewer) >>> >>> Thanks, Roger >>> >>> >>> >>> On 1/10/2014 1:31 AM, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan >>> >> >
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Yes, you are right. I just found this macro. It looks very handy to use. Thanks! -Dan On 01/10/2014 09:59 AM, roger riggs wrote: Hi Dan, One other comment, instead of changing the return type of the setStaticIntField just return and the caller should check for exceptions and return. See jni.h: CHECK_EXCEPTION(env) Roger On 1/10/2014 11:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
On 10 Jan 2014, at 18:05, Dan Xu wrote: > Hi Roger, > > My macro is a little different from yours, which compares with -1 instead of > NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are > useful when I cannot decide the pending exception state by just using return > values. > > As for the style, actually I prefer the (!pointer) to (pointer == NULL) > because it is more concise and also make me avoid the typo like (pointer = > NULL), which I cannot find from the compilation. Thanks! Not that it matters, but my preference is to == NULL. -Chris. > > -Dan > > > On 01/10/2014 08:40 AM, roger riggs wrote: >> Hi Dan, >> >> Just pushed are macros in jni_util.h to do the same function as your new >> macros. >> Please update to use the common macros instead of introducing new ones. >> >> Style wise, I would avoid mixing binary operators (!) with pointers. >> it is clearer to compare with NULL. (The CHECK_NULL macro will do the check >> and return). >> >> (Not a Reviewer) >> >> Thanks, Roger >> >> >> >> On 1/10/2014 1:31 AM, Dan Xu wrote: >>> Hi All, >>> >>> Please review the fix for JNI pending exception issues reported in >>> jdk-8029007. Thanks! >>> >>> Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 >>> >>> -Dan >> >
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Hi Roger, My macro is a little different from yours, which compares with -1 instead of NULL. I also see CHECK_EXCEPTION macro. Thanks for adding them, which are useful when I cannot decide the pending exception state by just using return values. As for the style, actually I prefer the (!pointer) to (pointer == NULL) because it is more concise and also make me avoid the typo like (pointer = NULL), which I cannot find from the compilation. Thanks! -Dan On 01/10/2014 08:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Hi Dan, One other comment, instead of changing the return type of the setStaticIntField just return and the caller should check for exceptions and return. See jni.h: CHECK_EXCEPTION(env) Roger On 1/10/2014 11:40 AM, roger riggs wrote: Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
On 01/10/2014 02:32 AM, Alan Bateman wrote: On 10/01/2014 06:31, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 This looks good, the only one that isn't clear (to me) is the GetStringLength usage in MessageUtil.c where I don't think it is possible to ever get < 0. This may be a case where you need to use ExceptionOccured instead. -Alan. According to jni.cpp, GetStringLength() will always return positive value or 0. For simplicity, I will change "<= 0" to "== 0". Thanks, Alan. -Dan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Thanks, Chris. Right, I don't find any usage of getThreadStateValues, so it is simpler to just remove it. -Dan On 01/09/2014 11:49 PM, Chris Hegarty wrote: Looks ok to me. I presume getThreadStateValues is simply no longer needed. -Chris. On 10 Jan 2014, at 06:31, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Hi Dan, Just pushed are macros in jni_util.h to do the same function as your new macros. Please update to use the common macros instead of introducing new ones. Style wise, I would avoid mixing binary operators (!) with pointers. it is clearer to compare with NULL. (The CHECK_NULL macro will do the check and return). (Not a Reviewer) Thanks, Roger On 1/10/2014 1:31 AM, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 -Dan
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
On 10/01/2014 06:31, Dan Xu wrote: Hi All, Please review the fix for JNI pending exception issues reported in jdk-8029007. Thanks! Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 This looks good, the only one that isn't clear (to me) is the GetStringLength usage in MessageUtil.c where I don't think it is possible to ever get < 0. This may be a case where you need to use ExceptionOccured instead. -Alan.
Re: RFR: JDK-8029007, Check src/share/native/sun/misc code for JNI pending exceptions
Looks ok to me. I presume getThreadStateValues is simply no longer needed. -Chris. > On 10 Jan 2014, at 06:31, Dan Xu wrote: > > Hi All, > > Please review the fix for JNI pending exception issues reported in > jdk-8029007. Thanks! > > Webrev: http://cr.openjdk.java.net/~dxu/8029007/webrev.00/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8029007 > > -Dan
