Tim Armstrong has posted comments on this change. Change subject: IMPALA-5327: Handle return of JNI GetStringUTFChar ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/7642/3/be/src/util/jni-util.cc File be/src/util/jni-util.cc: Line 186: env->ReleaseStringUTFChars(msg, msg_str); We're doing this on every exit path out of the function. How about using the ScopeExitTrigger utility? Or, even better, could we make a utility class that holds the UTF chars and automatically releases them in the destructor? Line 212: jboolean dummy; We may not need the dummy variable. The docs say: "If isCopy is not NULL, then *isCopy is set to JNI_TRUE if a copy is made; or it is set to JNI_FALSE if no copy is made." http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/functions.html Line 215: if (env->ExceptionCheck()) { Can put this conditional on one line. Line 218: if (result != nullptr) { Same here. Line 225: return Status::OK(); We can unnest the contents of this else. I.e. if (...) { ... return Status(fail_message); } return Status::OK(). We generally favour doing this in the codebase to get more concise code. http://gerrit.cloudera.org:8080/#/c/7642/3/be/src/util/jni-util.h File be/src/util/jni-util.h: Line 236: /// error Status. Should mention that the caller is required to free 'result' in a specific way. PS3, Line 237: const char*& Output argument should be a pointer instead of a reference: https://google.github.io/styleguide/cppguide.html#Reference_Arguments -- To view, visit http://gerrit.cloudera.org:8080/7642 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I47eed045f21c6ed3a8decf422ce8429fc66c4322 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
