[jira] [Commented] (IMPALA-7388) JNI THROW_IF_ERROR macros use local scope variables which likely conflict

["ASF subversion and git services (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/IMPALA-7388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16579252#comment-16579252 ] 

ASF subversion and git services commented on IMPALA-7388:
---------------------------------------------------------

Commit ea615d1d8026dbe1cda3f5c2b048d210b2df435b in impala's branch refs/heads/master from [~tlipcon]
[ https://git-wip-us.apache.org/repos/asf?p=impala.git;h=ea615d1 ]

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>


> JNI THROW_IF_ERROR macros use local scope variables which likely conflict
> -------------------------------------------------------------------------
>
>                 Key: IMPALA-7388
>                 URL: https://issues.apache.org/jira/browse/IMPALA-7388
>             Project: IMPALA
>          Issue Type: Bug
>            Reporter: Todd Lipcon
>            Assignee: Todd Lipcon
>            Priority: Major
>             Fix For: Impala 3.1.0
>
>
> The THROW_IF_ERROR macros all use a locally scoped variable "status". If they're called with a pattern like:
> {code:java}
> Status status = foo();
> THROW_IF_ERROR(status, ...){code}
>  
> then the status variable inside the macro ends up being assigned to itself instead of the outer-scope variable. This makes it not throw or return, which is quite surprising.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-all-unsubscribe@impala.apache.org
For additional commands, e-mail: issues-all-help@impala.apache.org