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 <tw...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to