Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11181 )
Change subject: IMPALA-7421. Static methods use wrong JNI call function ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/11181/1/be/src/util/jni-util.h File be/src/util/jni-util.h: http://gerrit.cloudera.org:8080/#/c/11181/1/be/src/util/jni-util.h@219 PS1, Line 219: JniCall Just an idea: the correct usage of the fluent interface could be enforced by the compiler. One way to do this is changing the members to protected and creating subclasses of JniCall like JniCallExpectArguments and JniCallExpectCall. Functions that are not callable on a freshly constructed JniCall could be moved there and the "next stage" could be specified by casting the return value to the correct subclass, e.g. JniCallExpectArguments& on_class(jclass cls) { DCHECK(!instance_ && !class_); class_ = DCHECK_NOTNULL(cls); return *(JniCallExpectArguments*)this; } http://gerrit.cloudera.org:8080/#/c/11181/1/be/src/util/jni-util.h@227 PS1, Line 227: JniCall& on_class(jclass cls) { WARN_UNUSED_RESULT could be added here and in other "fluent" functions. http://gerrit.cloudera.org:8080/#/c/11181/1/be/src/util/jni-util.h@234 PS1, Line 234: !instance_ && !instance_ The same thing is checked twice here. http://gerrit.cloudera.org:8080/#/c/11181/1/be/src/util/jni-util.h@455 PS1, Line 455: Sttring possible typo: String http://gerrit.cloudera.org:8080/#/c/11181/1/be/src/util/logging-support.cc File be/src/util/logging-support.cc: http://gerrit.cloudera.org:8080/#/c/11181/1/be/src/util/logging-support.cc@139 PS1, Line 139: return Status::OK(); As this is already modified the RETURN_IF_ERROR could be replaced with simply return. http://gerrit.cloudera.org:8080/#/c/11181/1/be/src/util/logging-support.cc@144 PS1, Line 144: return Status::OK(); Same as line 139. -- To view, visit http://gerrit.cloudera.org:8080/11181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7cde6ca91613b63afe5307f4d819fb24cb17fd6 Gerrit-Change-Number: 11181 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Fri, 10 Aug 2018 13:37:08 +0000 Gerrit-HasComments: Yes
