You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2022/05/10 03:19:17 UTC
[kudu-CR] [client] add DISALLOW COPY AND ASSIGN() for a few classes
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18511
Change subject: [client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
......................................................................
[client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
After making the copy constructor and the assignment operator for the
ResourceMetrics class private, I took a quick look at the other exported
classes in the Kudu C++ client API and added corresponding macro
where appropriate.
Strictly speaking, this change breaks the ABI compatibility for the
Kudu C++ client API, but a code that would use these privatized members
was unsafe anyways. I guess it's easier to fix the linker error after
upgrading to a newer version of the Kudu client library than have
a hidden memory corruption problem in a Kudu C++ application.
Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
---
M src/kudu/client/client.h
M src/kudu/client/schema.h
2 files changed, 7 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/18511/1
--
To view, visit http://gerrit.cloudera.org:8080/18511
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
Gerrit-Change-Number: 18511
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
[kudu-CR] [client] add DISALLOW COPY AND ASSIGN() for a few classes
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18511 )
Change subject: [client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18511
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
Gerrit-Change-Number: 18511
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 May 2022 17:35:46 +0000
Gerrit-HasComments: No
[kudu-CR] [client] add DISALLOW COPY AND ASSIGN() for a few classes
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18511 )
Change subject: [client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18511/1//COMMIT_MSG
Commit Message:
PS1:
We could use the special '= delete' syntax for the copy constructor and the assignment operator (that's available since C++11), but we are tied to be C++98 compatible in the Kudu C++ client API, unfortunately.
--
To view, visit http://gerrit.cloudera.org:8080/18511
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
Gerrit-Change-Number: 18511
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 May 2022 15:06:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] add DISALLOW COPY AND ASSIGN() for a few classes
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18511 )
Change subject: [client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
......................................................................
Patch Set 1:
> Looks good.
> I check all 'data_' with comment '// Owned' in client.h and
> schema.h.
> With this patch, they all either have copy constructor defined or
> DISALLOW_COPY_AND_ASSIGN macro declared.
>
> I wonder though, can we tell clang-tidy to check these kind of
> occurrences?
Yeah, that would be great if could have those automatic checks. However, I could not find anything which is quite close this time. I'm planning to spend a bit more time looking at something suitable: I'll let you know if I find anything relevant.
--
To view, visit http://gerrit.cloudera.org:8080/18511
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
Gerrit-Change-Number: 18511
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 May 2022 17:39:50 +0000
Gerrit-HasComments: No
[kudu-CR] [client] add DISALLOW COPY AND ASSIGN() for a few classes
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18511 )
Change subject: [client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
......................................................................
Patch Set 1: Code-Review+1
Looks good.
I check all 'data_' with comment '// Owned' in client.h and schema.h.
With this patch, they all either have copy constructor defined or DISALLOW_COPY_AND_ASSIGN macro declared.
I wonder though, can we tell clang-tidy to check these kind of occurrences?
--
To view, visit http://gerrit.cloudera.org:8080/18511
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
Gerrit-Change-Number: 18511
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 May 2022 16:57:51 +0000
Gerrit-HasComments: No
[kudu-CR] [client] add DISALLOW COPY AND ASSIGN() for a few classes
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18511 )
Change subject: [client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
......................................................................
Patch Set 1:
> Patch Set 1: Code-Review+2
Thanks a lot for quick review!
--
To view, visit http://gerrit.cloudera.org:8080/18511
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
Gerrit-Change-Number: 18511
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 10 May 2022 17:46:05 +0000
Gerrit-HasComments: No
[kudu-CR] [client] add DISALLOW COPY AND ASSIGN() for a few classes
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18511 )
Change subject: [client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
......................................................................
[client] add DISALLOW_COPY_AND_ASSIGN() for a few classes
After making the copy constructor and the assignment operator for the
ResourceMetrics class private, I took a quick look at the other exported
classes in the Kudu C++ client API and added corresponding macro
where appropriate.
Strictly speaking, this change breaks the ABI compatibility for the
Kudu C++ client API, but a code that would use these privatized members
was unsafe anyways. I guess it's easier to fix the linker error after
upgrading to a newer version of the Kudu client library than have
a hidden memory corruption problem in a Kudu C++ application.
Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
Reviewed-on: http://gerrit.cloudera.org:8080/18511
Tested-by: Kudu Jenkins
Reviewed-by: Riza Suminto <ri...@cloudera.com>
Reviewed-by: Attila Bukor <ab...@apache.org>
---
M src/kudu/client/client.h
M src/kudu/client/schema.h
2 files changed, 7 insertions(+), 0 deletions(-)
Approvals:
Kudu Jenkins: Verified
Riza Suminto: Looks good to me, but someone else must approve
Attila Bukor: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18511
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656
Gerrit-Change-Number: 18511
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>