You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by to...@apache.org on 2018/08/14 04:39:06 UTC
[2/2] impala git commit: IMPALA-7388. Fix issues in and optimize
various Status macros
IMPALA-7388. Fix issues in and optimize various Status macros
Prior to this patch, the JNI THROW_IF macros used a variable 'status' in
local scope to hold the result of the statement whose status is to be
checked. This could cause problems if there is another variable 'status'
in the external scope, as the inner scope would shadow the outer. For
example:
Status status = foo();
THROW_IF_ERROR(status, ...);
... would actually end up resulting in a macro expansion like:
do {
Status status = status;
...
} while (false);
Suprisingly, such code is legal C++ and ends up calling the Status copy
constructor with an uninitialized version of itself as the argument. gcc
doesn't seem to emit a warning for this construct, though clang does
emit "variable 'status' is uninitialized when used within its own
initialization".
If such code is emitted, we'll get undefined behavior and most likely
not end up throwing an error even if status is not OK.
The fix is simple: use a less-likely-to-conflict variable name _status
in the macro. This also fixes other macros that previously used
__status__ to use _status to align with the fact that __*__ style
keywords are reserved by the C++ standard.
Additionally, this patch changes some other similar macros to use const
references instead of creating new variables. On the surface, it might
look wrong to assign a const reference to a temporary such as the result
of a function call. However, C++ explicitly allows this and ensures that
the lifetime of the temporary is extended to the lifetime of the
captured reference, making it safe[1]. This acts as a small optimization
since the RETURN_IF_ERROR macros won't call their copy-constructor an
extra time.
[1] https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/
Change-Id: I3d62c99bbd83cc290f5992077258af156eaa6330
Reviewed-on: http://gerrit.cloudera.org:8080/11153
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/ea615d1d
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/ea615d1d
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/ea615d1d
Branch: refs/heads/master
Commit: ea615d1d8026dbe1cda3f5c2b048d210b2df435b
Parents: 884fcf8
Author: Todd Lipcon <to...@cloudera.com>
Authored: Thu Aug 2 01:20:18 2018 -0700
Committer: Todd Lipcon <to...@apache.org>
Committed: Tue Aug 14 04:14:29 2018 +0000
----------------------------------------------------------------------
be/src/common/status.h | 10 +++++-----
be/src/exec/kudu-util.h | 2 +-
be/src/service/impala-beeswax-server.cc | 6 +++---
be/src/service/impala-hs2-server.cc | 9 +++++----
be/src/util/jni-util.h | 18 +++++++++---------
5 files changed, 23 insertions(+), 22 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/ea615d1d/be/src/common/status.h
----------------------------------------------------------------------
diff --git a/be/src/common/status.h b/be/src/common/status.h
index 24dba8b..b211376 100644
--- a/be/src/common/status.h
+++ b/be/src/common/status.h
@@ -288,8 +288,8 @@ std::ostream& operator<<(std::ostream& os, const Status& status);
/// some generally useful macros
#define RETURN_IF_ERROR(stmt) \
do { \
- ::impala::Status __status__ = (stmt); \
- if (UNLIKELY(!__status__.ok())) return __status__; \
+ const ::impala::Status& _status = (stmt); \
+ if (UNLIKELY(!_status.ok())) return _status; \
} while (false)
#define RETURN_VOID_IF_ERROR(stmt) \
@@ -299,9 +299,9 @@ std::ostream& operator<<(std::ostream& os, const Status& status);
#define ABORT_IF_ERROR(stmt) \
do { \
- ::impala::Status __status__ = (stmt); \
- if (UNLIKELY(!__status__.ok())) { \
- ABORT_WITH_ERROR(__status__.GetDetail()); \
+ const ::impala::Status& _status = (stmt); \
+ if (UNLIKELY(!_status.ok())) { \
+ ABORT_WITH_ERROR(_status.GetDetail()); \
} \
} while (false)
http://git-wip-us.apache.org/repos/asf/impala/blob/ea615d1d/be/src/exec/kudu-util.h
----------------------------------------------------------------------
diff --git a/be/src/exec/kudu-util.h b/be/src/exec/kudu-util.h
index 1d9a9bd..8ed25a9 100644
--- a/be/src/exec/kudu-util.h
+++ b/be/src/exec/kudu-util.h
@@ -35,7 +35,7 @@ namespace impala {
/// Evaluates the prepend argument only if the status is not OK.
#define KUDU_RETURN_IF_ERROR(expr, prepend) \
do { \
- kudu::Status _s = (expr); \
+ const kudu::Status& _s = (expr); \
if (UNLIKELY(!_s.ok())) { \
return Status(strings::Substitute("$0: $1", prepend, _s.ToString())); \
} \
http://git-wip-us.apache.org/repos/asf/impala/blob/ea615d1d/be/src/service/impala-beeswax-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-beeswax-server.cc b/be/src/service/impala-beeswax-server.cc
index 49d0b8c..3ecbe80 100644
--- a/be/src/service/impala-beeswax-server.cc
+++ b/be/src/service/impala-beeswax-server.cc
@@ -42,9 +42,9 @@ using namespace beeswax;
#define RAISE_IF_ERROR(stmt, ex_type) \
do { \
- Status __status__ = (stmt); \
- if (UNLIKELY(!__status__.ok())) { \
- RaiseBeeswaxException(__status__.GetDetail(), ex_type); \
+ const Status& _status = (stmt); \
+ if (UNLIKELY(!_status.ok())) { \
+ RaiseBeeswaxException(_status.GetDetail(), ex_type); \
} \
} while (false)
http://git-wip-us.apache.org/repos/asf/impala/blob/ea615d1d/be/src/service/impala-hs2-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-hs2-server.cc b/be/src/service/impala-hs2-server.cc
index edcc1c6..c49c65b 100644
--- a/be/src/service/impala-hs2-server.cc
+++ b/be/src/service/impala-hs2-server.cc
@@ -66,15 +66,16 @@ const TProtocolVersion::type MAX_SUPPORTED_HS2_VERSION =
#define HS2_RETURN_ERROR(return_val, error_msg, error_state) \
do { \
return_val.status.__set_statusCode(thrift::TStatusCode::ERROR_STATUS); \
- return_val.status.__set_errorMessage(error_msg); \
- return_val.status.__set_sqlState(error_state); \
+ return_val.status.__set_errorMessage((error_msg)); \
+ return_val.status.__set_sqlState((error_state)); \
return; \
} while (false)
#define HS2_RETURN_IF_ERROR(return_val, status, error_state) \
do { \
- if (UNLIKELY(!status.ok())) { \
- HS2_RETURN_ERROR(return_val, status.GetDetail(), error_state); \
+ const Status& _status = (status); \
+ if (UNLIKELY(!_status.ok())) { \
+ HS2_RETURN_ERROR(return_val, _status.GetDetail(), (error_state)); \
return; \
} \
} while (false)
http://git-wip-us.apache.org/repos/asf/impala/blob/ea615d1d/be/src/util/jni-util.h
----------------------------------------------------------------------
diff --git a/be/src/util/jni-util.h b/be/src/util/jni-util.h
index 0176a13..fd87839 100644
--- a/be/src/util/jni-util.h
+++ b/be/src/util/jni-util.h
@@ -28,29 +28,29 @@
#define THROW_IF_ERROR_WITH_LOGGING(stmt, env, adaptor) \
do { \
- Status status = (stmt); \
- if (!status.ok()) { \
+ const Status& _status = (stmt); \
+ if (!_status.ok()) { \
(adaptor)->WriteErrorLog(); \
(adaptor)->WriteFileErrors(); \
- (env)->ThrowNew((adaptor)->impala_exc_cl(), status.GetDetail().c_str()); \
+ (env)->ThrowNew((adaptor)->impala_exc_cl(), _status.GetDetail().c_str()); \
return; \
} \
} while (false)
#define THROW_IF_ERROR(stmt, env, impala_exc_cl) \
do { \
- Status status = (stmt); \
- if (!status.ok()) { \
- (env)->ThrowNew((impala_exc_cl), status.GetDetail().c_str()); \
+ const Status& _status = (stmt); \
+ if (!_status.ok()) { \
+ (env)->ThrowNew((impala_exc_cl), _status.GetDetail().c_str()); \
return; \
} \
} while (false)
#define THROW_IF_ERROR_RET(stmt, env, impala_exc_cl, ret) \
do { \
- Status status = (stmt); \
- if (!status.ok()) { \
- (env)->ThrowNew((impala_exc_cl), status.GetDetail().c_str()); \
+ const Status& _status = (stmt); \
+ if (!_status.ok()) { \
+ (env)->ThrowNew((impala_exc_cl), _status.GetDetail().c_str()); \
return (ret); \
} \
} while (false)