Skip to content

Conversation

xxDark
Copy link

@xxDark xxDark commented Jan 10, 2025

Attach pending exception as the cause in throwByName if possible. This change makes it easier to debug errors.

Copy link
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review. In general the idea makes sense. I left an inline comment, that shows how a simpler version could look from my POV.

What is crucially missing is a unittest, that verifies that this works. Please add a demonstrator to validate.

Comment on lines 426 to 469
if (cls != NULL) { /* Otherwise an exception has already been thrown */
if (thrown != NULL) {
/* Attach thrown exception if possible. */
/* do_throw_clean is an exit path that will delete 'thrown' if we can't do that. */
jmethodID init = (*env)->GetMethodID(env, cls, "<init>", "(Ljava/lang/String;)V");
if (init == NULL) {
goto do_throw_clean;
}
jmethodID initCause = (*env)->GetMethodID(env, cls, "initCause", "(Ljava/lang/Throwable;)Ljava/lang/Throwable;");
if (initCause == NULL) {
goto do_throw_clean;
}
jobject str = (*env)->NewStringUTF(env, msg);
if (str == NULL) {
goto do_throw_clean;
}
jthrowable t = (jthrowable) (*env)->NewObject(env, cls, init, str);
(*env)->DeleteLocalRef(env, str);
if (t == NULL) {
goto do_throw_clean;
}
jobject ret = (*env)->CallObjectMethod(env, t, initCause, thrown);
if (ret == NULL) {
goto do_throw_clean;
}
(*env)->DeleteLocalRef(env, ret);
(*env)->Throw(env, t);
goto clean_cls;
} else {
goto do_throw;
}
/* Deletes thrown exception reference and clears the exception. */
do_throw_clean:
(*env)->DeleteLocalRef(env, thrown);
(*env)->ExceptionClear(env);
do_throw:
(*env)->ThrowNew(env, cls, msg);

clean_cls:
/* It's a good practice to clean up the local references. */
(*env)->DeleteLocalRef(env, cls);
} else if (thrown != NULL) {
(*env)->DeleteLocalRef(env, thrown);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the cases are to complex and can be simplified to:

Suggested change
if (cls != NULL) { /* Otherwise an exception has already been thrown */
if (thrown != NULL) {
/* Attach thrown exception if possible. */
/* do_throw_clean is an exit path that will delete 'thrown' if we can't do that. */
jmethodID init = (*env)->GetMethodID(env, cls, "<init>", "(Ljava/lang/String;)V");
if (init == NULL) {
goto do_throw_clean;
}
jmethodID initCause = (*env)->GetMethodID(env, cls, "initCause", "(Ljava/lang/Throwable;)Ljava/lang/Throwable;");
if (initCause == NULL) {
goto do_throw_clean;
}
jobject str = (*env)->NewStringUTF(env, msg);
if (str == NULL) {
goto do_throw_clean;
}
jthrowable t = (jthrowable) (*env)->NewObject(env, cls, init, str);
(*env)->DeleteLocalRef(env, str);
if (t == NULL) {
goto do_throw_clean;
}
jobject ret = (*env)->CallObjectMethod(env, t, initCause, thrown);
if (ret == NULL) {
goto do_throw_clean;
}
(*env)->DeleteLocalRef(env, ret);
(*env)->Throw(env, t);
goto clean_cls;
} else {
goto do_throw;
}
/* Deletes thrown exception reference and clears the exception. */
do_throw_clean:
(*env)->DeleteLocalRef(env, thrown);
(*env)->ExceptionClear(env);
do_throw:
(*env)->ThrowNew(env, cls, msg);
clean_cls:
/* It's a good practice to clean up the local references. */
(*env)->DeleteLocalRef(env, cls);
} else if (thrown != NULL) {
(*env)->DeleteLocalRef(env, thrown);
}
if (cls != NULL) { /* Otherwise an exception has already been thrown */
jthrowable t = NULL;
jmethodID init = (*env)->GetMethodID(env, cls, "<init>", "(Ljava/lang/String;)V");
jobject str = (*env)->NewStringUTF(env, msg);
if (init != NULL && str != NULL) {
t = (jthrowable) (*env)->NewObject(env, cls, init, str);
}
if (str != NULL) {
(*env)->DeleteLocalRef(env, str);
}
if (thrown != NULL && t != NULL) {
jmethodID initCause = (*env)->GetMethodID(env, cls, "initCause", "(Ljava/lang/Throwable;)Ljava/lang/Throwable;");
/* Attach thrown exception if possible. */
jobject ret = (*env)->CallObjectMethod(env, t, initCause, thrown);
(*env)->DeleteLocalRef(env, ret);
}
(*env)->Throw(env, t);
(*env)->DeleteLocalRef(env, cls);
}
if (thrown != NULL) {
(*env)->DeleteLocalRef(env, thrown);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants