Hi, David.
Thank you for review, here are my thoughts.
The patch uses the same pattern as NULL_CHECK0.
If an exception is occurred then NULL_CHECK0 in one situation and
CHECK_EXCEPTION_RETURN_VALUE in another situation will return 0 to the
upper code which will process this value(there is the same 0/null checks).
The difference between NULL_CHECK0 and CHECK_EXCEPTION_RETURN_VALUE, is
that the first is used when the "NULL" result also means some error, for
example:
NULL_CHECK0(mid = (*env)->GetStaticMethodID(env, cls,
"getApplicationClass",
"()Ljava/lang/Class;"));
appClass = (*env)->CallStaticObjectMethod(env, cls, mid);
CHECK_EXCEPTION_RETURN_VALUE(0);
In the code above the GetStaticMethodID() returns NULL if the operation
fails, but it also throw an exceptions like NoSuchMethodError/etc.
But in case of CallStaticObjectMethod() we cannot check for NULL which
can be a valid result, so I added a check for exception and return 0.
This value will be propagated to JavaMain() and I as far as understand
will stop the execution.
On 9/12/17 13:56, David Holmes wrote:
Hi Sergey,
On 13/09/2017 5:18 AM, Sergey Bylokhov wrote:
Hello,
Please review the fix for jdk10/jdk10.
Bug: https://bugs.openjdk.java.net/browse/JDK-8187442
Webrev can be found at:
http://cr.openjdk.java.net/~serb/8187442/webrev.00
This doesn't look right to me:
str = (*env)->CallStaticObjectMethod(env, cls,
makePlatformStringMID, USE_STDERR, ary);
+ CHECK_EXCEPTION_RETURN_VALUE(0);
(*env)->DeleteLocalRef(env, ary);
return str;
If there is an exception pending you fail to delete the local ref. And
there's no need to clear the exception before calling DeleteLocalRef as
that is okay to call with a pending exception. But there is no point
returning zero if an exception occurred because in that case str will
already be 0/NULL.
The same here:
1596 appClass = (*env)->CallStaticObjectMethod(env, cls, mid);
1597 CHECK_EXCEPTION_RETURN_VALUE(0);
1598 return appClass;
If an exception is pending then appClass will be 0/NULL.
In addition CHECK_EXCEPTION_RETURN_VALUE doesn't clear the pending
exception so I can't see what warnings this would be clearing up ???
Thanks,
David
-----
The simple application with empty main method produce some "warnings"
when Xcheck:jni is used. Because of that it is hard to cleanup other
parts of jdk from such warnings.
Two additional checks were added, in both cases the new code will
return 0 in the same way as NULL_CHECK0 in the same methods.
--
Best regards, Sergey.
