You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2020/03/28 09:11:21 UTC
[kudu-CR] gutil: remove callback and bind from the codebase
Hello Alexey Serbin, Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15583
to review the following change.
Change subject: gutil: remove callback and bind from the codebase
......................................................................
gutil: remove callback and bind from the codebase
It is fitting that 5.5 years after bringing this code into Kudu (see commit
31f072096), I'm lucky enough to be the one burying it. There are no more
usages of this functionality; everything has been migrated to std::function
with lambdas for binding/capturing.
Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
---
M build-support/release/rat_exclude_files.txt
M docs/contributing.adoc
M src/kudu/gutil/CMakeLists.txt
D src/kudu/gutil/bind.h
D src/kudu/gutil/bind.h.pump
D src/kudu/gutil/bind_helpers.h
D src/kudu/gutil/bind_internal.h
D src/kudu/gutil/bind_internal.h.pump
D src/kudu/gutil/callback.h
D src/kudu/gutil/callback.h.pump
D src/kudu/gutil/callback_forward.h
D src/kudu/gutil/callback_internal.cc
D src/kudu/gutil/callback_internal.h
D src/kudu/gutil/raw_scoped_refptr_mismatch_checker.h
M src/kudu/util/CMakeLists.txt
D src/kudu/util/callback_bind-test.cc
16 files changed, 6 insertions(+), 6,046 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/15583/1
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc
File docs/contributing.adoc:
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc@327
PS4, Line 327: {cpp} 14
As Andrew pointed, probably {cpp}14 would be the better way of writing it in here.
Sometimes it's formatted as '{cpp} 11' as in 'Notes on {cpp} 11', but it I agree with Andrew that using '{cpp}11' would be more appropriate.
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc@329
PS4, Line 329: {cpp} 11
ditto
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 30 Mar 2020 20:08:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 2: Verified+1
Overriding Jenkins, known race.
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 29 Mar 2020 21:18:04 +0000
Gerrit-HasComments: No
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15583
to look at the new patch set (#5).
Change subject: gutil: remove callback and bind from the codebase
......................................................................
gutil: remove callback and bind from the codebase
It is fitting that 5.5 years after bringing this code into Kudu (see commit
31f072096), I'm lucky enough to be the one burying it. There are no more
usages of this functionality; everything has been migrated to std::function
with lambdas for binding/capturing.
Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
---
M build-support/release/rat_exclude_files.txt
M docs/contributing.adoc
M src/kudu/gutil/CMakeLists.txt
D src/kudu/gutil/bind.h
D src/kudu/gutil/bind.h.pump
D src/kudu/gutil/bind_helpers.h
D src/kudu/gutil/bind_internal.h
D src/kudu/gutil/bind_internal.h.pump
D src/kudu/gutil/callback.h
D src/kudu/gutil/callback.h.pump
D src/kudu/gutil/callback_forward.h
D src/kudu/gutil/callback_internal.cc
D src/kudu/gutil/callback_internal.h
D src/kudu/gutil/raw_scoped_refptr_mismatch_checker.h
M src/kudu/util/CMakeLists.txt
D src/kudu/util/callback_bind-test.cc
16 files changed, 11 insertions(+), 6,046 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/15583/5
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15583/2/docs/contributing.adoc
File docs/contributing.adoc:
http://gerrit.cloudera.org:8080/#/c/15583/2/docs/contributing.adoc@320
PS2, Line 320: All code should use C++11 lambdas to capture and manage functors. Functions that
: take a lambda as an argument should use `std::function` as the argument's type.
> +1
The ref-counting points are still valid. Having reviewed my changes, I bet you saw things like:
std::function<void()> Foo::CreateLambda() {
scoped_refptr<Foo> self(this);
return [self]() { self->DoSomething(); }
}
This code adds a ref to Foo, adds another ref to Foo when 'self' is copied into the lambda, drops a ref from Foo when CreateLambda() returns, and drops another ref from Foo later on, when the lambda goes out of scope and is destroyed.
In C++14: we'll be able to avoid the extra ref I think:
std::function<void()> Foo::CreateLambda() {
return [self = scoped_refptr<Foo>(this)]() { self->DoSomething(); }
}
Anyway, kudu::Bind did that stuff automatically for us, but I don't think that alone was a reason to keep using it.
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Mar 2020 18:55:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15583
to look at the new patch set (#4).
Change subject: gutil: remove callback and bind from the codebase
......................................................................
gutil: remove callback and bind from the codebase
It is fitting that 5.5 years after bringing this code into Kudu (see commit
31f072096), I'm lucky enough to be the one burying it. There are no more
usages of this functionality; everything has been migrated to std::function
with lambdas for binding/capturing.
Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
---
M build-support/release/rat_exclude_files.txt
M docs/contributing.adoc
M src/kudu/gutil/CMakeLists.txt
D src/kudu/gutil/bind.h
D src/kudu/gutil/bind.h.pump
D src/kudu/gutil/bind_helpers.h
D src/kudu/gutil/bind_internal.h
D src/kudu/gutil/bind_internal.h.pump
D src/kudu/gutil/callback.h
D src/kudu/gutil/callback.h.pump
D src/kudu/gutil/callback_forward.h
D src/kudu/gutil/callback_internal.cc
D src/kudu/gutil/callback_internal.h
D src/kudu/gutil/raw_scoped_refptr_mismatch_checker.h
M src/kudu/util/CMakeLists.txt
D src/kudu/util/callback_bind-test.cc
16 files changed, 11 insertions(+), 6,046 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/15583/4
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 30 Mar 2020 23:24:03 +0000
Gerrit-HasComments: No
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 4: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc
File docs/contributing.adoc:
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc@318
PS4, Line 318: ==== Function Binding and Callbacks
In other areas of this doc, we use "{cpp}11" with no space, which seems more correct.
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc@320
PS4, Line 320: All code should use {cpp} 11 lambdas to capture and manage functors. Functions that
: take a lambda as an argument should use `std::function` as the argument's
: type. Do not use boost::bind or std::bind to create functors. Lambdas offer the
: compiler greater opportunity to inline, and std::bind in particular is
: link:https://abseil.io/tips/108[error-prone] and has a proclivity towards heap
: allocation for storing bound parameters.
:
nit: use backticks for the `bind` references?
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 30 Mar 2020 19:14:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15583
to look at the new patch set (#2).
Change subject: gutil: remove callback and bind from the codebase
......................................................................
gutil: remove callback and bind from the codebase
It is fitting that 5.5 years after bringing this code into Kudu (see commit
31f072096), I'm lucky enough to be the one burying it. There are no more
usages of this functionality; everything has been migrated to std::function
with lambdas for binding/capturing.
Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
---
M build-support/release/rat_exclude_files.txt
M docs/contributing.adoc
M src/kudu/gutil/CMakeLists.txt
D src/kudu/gutil/bind.h
D src/kudu/gutil/bind.h.pump
D src/kudu/gutil/bind_helpers.h
D src/kudu/gutil/bind_internal.h
D src/kudu/gutil/bind_internal.h.pump
D src/kudu/gutil/callback.h
D src/kudu/gutil/callback.h.pump
D src/kudu/gutil/callback_forward.h
D src/kudu/gutil/callback_internal.cc
D src/kudu/gutil/callback_internal.h
D src/kudu/gutil/raw_scoped_refptr_mismatch_checker.h
M src/kudu/util/CMakeLists.txt
D src/kudu/util/callback_bind-test.cc
16 files changed, 6 insertions(+), 6,046 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/15583/2
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
gutil: remove callback and bind from the codebase
It is fitting that 5.5 years after bringing this code into Kudu (see commit
31f072096), I'm lucky enough to be the one burying it. There are no more
usages of this functionality; everything has been migrated to std::function
with lambdas for binding/capturing.
Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Reviewed-on: http://gerrit.cloudera.org:8080/15583
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M build-support/release/rat_exclude_files.txt
M docs/contributing.adoc
M src/kudu/gutil/CMakeLists.txt
D src/kudu/gutil/bind.h
D src/kudu/gutil/bind.h.pump
D src/kudu/gutil/bind_helpers.h
D src/kudu/gutil/bind_internal.h
D src/kudu/gutil/bind_internal.h.pump
D src/kudu/gutil/callback.h
D src/kudu/gutil/callback.h.pump
D src/kudu/gutil/callback_forward.h
D src/kudu/gutil/callback_internal.cc
D src/kudu/gutil/callback_internal.h
D src/kudu/gutil/raw_scoped_refptr_mismatch_checker.h
M src/kudu/util/CMakeLists.txt
D src/kudu/util/callback_bind-test.cc
16 files changed, 11 insertions(+), 6,046 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15583/2/docs/contributing.adoc
File docs/contributing.adoc:
http://gerrit.cloudera.org:8080/#/c/15583/2/docs/contributing.adoc@320
PS2, Line 320: All code should use C++11 lambdas to capture and manage functors. Functions that
: take a lambda as an argument should use `std::function` as the argument's type.
nit: maybe explicitly state we prefer lambdas over std::bind, and mention why? Are the points about ref-counting no longer a consideration?
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Mar 2020 06:14:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 31 Mar 2020 00:18:34 +0000
Gerrit-HasComments: No
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc
File docs/contributing.adoc:
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc@318
PS4, Line 318: ==== Function Binding and Callbacks
> In other areas of this doc, we use "{cpp}11" with no space, which seems mor
Done
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc@320
PS4, Line 320: All code should use {cpp} 11 lambdas to capture and manage functors. Functions that
: take a lambda as an argument should use `std::function` as the argument's
: type. Do not use boost::bind or std::bind to create functors. Lambdas offer the
: compiler greater opportunity to inline, and std::bind in particular is
: link:https://abseil.io/tips/108[error-prone] and has a proclivity towards heap
: allocation for storing bound parameters.
:
> nit: use backticks for the `bind` references?
Done
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc@327
PS4, Line 327: {cpp} 14
> As Andrew pointed, probably {cpp}14 would be the better way of writing it i
Done
http://gerrit.cloudera.org:8080/#/c/15583/4/docs/contributing.adoc@329
PS4, Line 329: {cpp} 11
> ditto
Done
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 30 Mar 2020 20:18:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] gutil: remove callback and bind from the codebase
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15583 )
Change subject: gutil: remove callback and bind from the codebase
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15583/2/docs/contributing.adoc
File docs/contributing.adoc:
http://gerrit.cloudera.org:8080/#/c/15583/2/docs/contributing.adoc@320
PS2, Line 320: All code should use C++11 lambdas to capture and manage functors. Functions that
: take a lambda as an argument should use `std::function` as the argument's type.
> nit: maybe explicitly state we prefer lambdas over std::bind, and mention w
+1
--
To view, visit http://gerrit.cloudera.org:8080/15583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib5fb53730f9c33b37415b22fcc775266467116b9
Gerrit-Change-Number: 15583
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Mar 2020 06:50:22 +0000
Gerrit-HasComments: Yes