You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/09/12 04:37:47 UTC
[kudu-CR] util: avoid redundant copies in various macros
Hello Dan Burkert, Mike Percy, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4379
to review the following change.
Change subject: util: avoid redundant copies in various macros
......................................................................
util: avoid redundant copies in various macros
A lot of macros have the form:
#define FOO(arg) \
Foo arg_ = (arg); \
... do something with arg_ ...
Of course the purpose of the 'arg_' local is to avoid double-evaluation
of the argument in the case that it's something expensive or something
with a side effect. However, this pattern results in an extra copy of
'arg' in the case that it's actually a local, eg in the pattern:
Foo f = ...;
FOO(f);
This expands out to a redundant copy 'Foo arg_ = f;'.
It turns out that the solution is to make 'Foo arg_' a const reference
'const Foo& arg_'. At first, this seems wrong: references to a temporary
are often bad. However, it turns out that C++ has a rule that const
references to temporaries on the stack keep that temporary alive as long as
the reference stays in scope. So, this is safe, and avoids an extra
copy.
Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
---
M src/kudu/util/status.h
M src/kudu/util/test_macros.h
2 files changed, 13 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/4379/1
--
To view, visit http://gerrit.cloudera.org:8080/4379
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] util: avoid redundant copies in various macros
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: util: avoid redundant copies in various macros
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4379
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] util: avoid redundant copies in various macros
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: util: avoid redundant copies in various macros
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/3364/
--
To view, visit http://gerrit.cloudera.org:8080/4379
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-HasComments: No
[kudu-CR] util: avoid redundant copies in various macros
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: util: avoid redundant copies in various macros
......................................................................
util: avoid redundant copies in various macros
A lot of macros have the form:
#define FOO(arg) \
Foo arg_ = (arg); \
... do something with arg_ ...
Of course the purpose of the 'arg_' local is to avoid double-evaluation
of the argument in the case that it's something expensive or something
with a side effect. However, this pattern results in an extra copy of
'arg' in the case that it's actually a local, eg in the pattern:
Foo f = ...;
FOO(f);
This expands out to a redundant copy 'Foo arg_ = f;'.
It turns out that the solution is to make 'Foo arg_' a const reference
'const Foo& arg_'. At first, this seems wrong: references to a temporary
are often bad. However, it turns out that C++ has a rule that const
references to temporaries on the stack keep that temporary alive as long as
the reference stays in scope. So, this is safe, and avoids an extra
copy.
Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Reviewed-on: http://gerrit.cloudera.org:8080/4379
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/util/status.h
M src/kudu/util/test_macros.h
2 files changed, 13 insertions(+), 13 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4379
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6057751d5c6790b2d350e77636b58079e2093cb7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>