[src] Elevenize KALDI_DISALLOW_COPY_AND_ASSIGN() #4547
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before C++11 we declared the same members private, usually at the very end of a class declaration. With standardization of the delete keyword and adoption of the pattern, it is now idiomatic to make deleted members public. The rationale is that deletion of copy members is part of the public behavior of a type, namely its uncopyability, and should be expressed in and readable from the public: section alone. Google coding style, which we are generally following, does not mention the 'DISALLOW_COPY_AND_ASSIGN' macro any more, and recommends using the '= delete;' construct directly. From https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types: > Every class's public interface must make clear which copy and move > operations the class supports. This should usually take the form of > explicitly declaring and/or deleting the appropriate operations in > the public section of the declaration. Examples from the guide use the delete keyword straight without any macro, but I think we should make an exception and continue using the macro, which is more self-explanatory. clang-tidy gets angry at non-public deleted members, too.
|
LGTM
…On Fri, May 28, 2021 at 8:28 AM kkm000 ***@***.***> wrote:
@kkm000 <https://github.com/kkm000> requested your review on: #4547
<#4547> [src] Elevenize
KALDI_DISALLOW_COPY_AND_ASSIGN().
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#4547 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACUKYXY4N5YUTTCYGZAZX3TTP6D5ZANCNFSM45WQMDMQ>
.
|
|
CI is green, merging, thanks for looking! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before C++11 we declared the same members private, usually at the very end of a class declaration. With standardization of the
deletekeyword and adoption of the pattern, it is now idiomatic to make deleted members public. The rationale (which I'm totally with) is that deletion of copy members is part of the public behavior of a type, namely its uncopyability, and should be expressed in and readable from thepublic:section alone.Google coding style, which we are generally following, does not mention the
DISALLOW_COPY_AND_ASSIGNmacro any more, and recommends using the= delete;construct directly:[Bold mine; the focus on public is very explicit.]
Examples from the guide use the
deletestraight without the macro¹, but I think we should make an exception and continue using the macro, which is more self-explanatory.clang-tidy gets angry at non-public deleted members, too.
_ _
¹ Which originated at Google as
DISALLOW_EVIL_CONSTRUCTORSand had been renamedDISALLOW_COPY_AND_ASSIGNduring my tenure, around 2014, when Google first started releasing parts of its code as OSS.