Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

sbenza added inline comments. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63 @@ +62,3 @@ + + Finder->addMatcher(returnStmt(IsBadReturnStatement, hasAncestor(IsGoodAssign)) + .bind("returnStmt"), baloghadamsoftware wrote: > sbenza

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added a comment. misc-unconventional-assign-operator, misc-assign-operator-conventions, misc-non-idiomatic-assign-operator or something else then? http://reviews.llvm.org/D18265 ___ cfe-commits mailing list

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added inline comments. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63 @@ +62,3 @@ + + Finder->addMatcher(returnStmt(IsBadReturnStatement, hasAncestor(IsGoodAssign)) + .bind("returnStmt"), sbenza wrote: >

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

sbenza added inline comments. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63 @@ +62,3 @@ + + Finder->addMatcher(returnStmt(IsBadReturnStatement, hasAncestor(IsGoodAssign)) + .bind("returnStmt"), baloghadamsoftware wrote: > sbenza

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added inline comments. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63 @@ +62,3 @@ + + Finder->addMatcher(returnStmt(IsBadReturnStatement, hasAncestor(IsGoodAssign)) + .bind("returnStmt"), sbenza wrote: >

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

sbenza added inline comments. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63 @@ +62,3 @@ + + Finder->addMatcher(returnStmt(IsBadReturnStatement, hasAncestor(IsGoodAssign)) + .bind("returnStmt"), baloghadamsoftware wrote: > sbenza

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added a comment. And what about the final name? http://reviews.llvm.org/D18265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added inline comments. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63 @@ +62,3 @@ + + Finder->addMatcher(returnStmt(IsBadReturnStatement, hasAncestor(IsGoodAssign)) + .bind("returnStmt"), sbenza wrote: > I

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

aaron.ballman added inline comments. Comment at: test/clang-tidy/misc-assign-operator.cpp:16 @@ +15,3 @@ + AlsoGood& operator=(AlsoGood); +}; + sbenza wrote: > This is a very common C++98 way of implementing copy-and-swap with copy > elision support. > You do:

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

sbenza added inline comments. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:63 @@ +62,3 @@ + + Finder->addMatcher(returnStmt(IsBadReturnStatement, hasAncestor(IsGoodAssign)) + .bind("returnStmt"), I dislike these uses of

RE: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return I agree with your point; that's why my slight preference is for leaving them split into multiple checks. Either we want this to be the catch-all for operator assignment checks (and plan to use config options to control behavior

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added a comment. In http://reviews.llvm.org/D18265#387078, @LegalizeAdulthood wrote: > Please summarize this check in `docs/ReleaseNotes.rst`. OK, I will do it after the name is decided. http://reviews.llvm.org/D18265 ___

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

LegalizeAdulthood added a subscriber: LegalizeAdulthood. LegalizeAdulthood added a comment. Please summarize this check in `docs/ReleaseNotes.rst`. There is a pending review that will stub this out automatically, but it hasn't been approved yet http://reviews.llvm.org/D18265

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

On Wed, Mar 30, 2016 at 9:23 AM, Alexander Kornienko wrote: > alexfh added a comment. > > In http://reviews.llvm.org/D18265#386720, @baloghadamsoftware wrote: > >> Actually, there was nothing wrong with assign operator signatures per se >> either although the original name of

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added a comment. misc-unconventional-assign-operator? http://reviews.llvm.org/D18265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

alexfh added a comment. In http://reviews.llvm.org/D18265#386720, @baloghadamsoftware wrote: > Actually, there was nothing wrong with assign operator signatures per se > either although the original name of the checker was AssignOperatorSignature. > The only change here is that it does not

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

On Wed, Mar 30, 2016 at 8:52 AM, Gábor Horváth wrote: > xazax.hun added a comment. > > In http://reviews.llvm.org/D18265#386717, @aaron.ballman wrote: > >> In http://reviews.llvm.org/D18265#386676, @alexfh wrote: >> >> > > What about NonIdiomaticAddignOperator or

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

On Wed, Mar 30, 2016 at 8:56 AM, Alexander Kornienko wrote: > alexfh added a comment. > > In http://reviews.llvm.org/D18265#386717, @aaron.ballman wrote: > >> In http://reviews.llvm.org/D18265#386676, @alexfh wrote: >> >> > > What about NonIdiomaticAddignOperator or

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added a comment. Actually, there was nothing wrong with assign operator signatures per se either although the original name of the checker was AssignOperatorSignature. The only change here is that it does not check the signature only anymore, but also the body (if present).

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

xazax.hun added a comment. In http://reviews.llvm.org/D18265#386717, @aaron.ballman wrote: > In http://reviews.llvm.org/D18265#386676, @alexfh wrote: > > > > What about NonIdiomaticAddignOperator or UnchainableAssignOperator? > > > > > > Yep, "unchainable" doesn't cover all problems the check

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

alexfh added a comment. In http://reviews.llvm.org/D18265#386717, @aaron.ballman wrote: > In http://reviews.llvm.org/D18265#386676, @alexfh wrote: > > > > What about NonIdiomaticAddignOperator or UnchainableAssignOperator? > > > > > > Yep, "unchainable" doesn't cover all problems the check

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

aaron.ballman added a comment. In http://reviews.llvm.org/D18265#386676, @alexfh wrote: > > What about NonIdiomaticAddignOperator or UnchainableAssignOperator? > > > Yep, "unchainable" doesn't cover all problems the check detects. > `misc-non-idiomatic-assign-operator` seems good though. I'd

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

alexfh added a comment. > What about NonIdiomaticAddignOperator or UnchainableAssignOperator? Yep, "unchainable" doesn't cover all problems the check detects. `misc-non-idiomatic-assign-operator` seems good though. I'd wait for the original author to chime in before making the change.

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added a comment. Unchainable is not enough: the original checker (which was extended) also checks for parameters and other qualifiers such as const or virtual. http://reviews.llvm.org/D18265 ___ cfe-commits mailing list

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

xazax.hun added a comment. What about NonIdiomaticAddignOperator or UnchainableAssignOperator? http://reviews.llvm.org/D18265 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware marked an inline comment as done. Comment at: clang-tidy/misc/MiscTidyModule.cpp:54 @@ -55,1 +53,3 @@ +CheckFactories.registerCheck( +"misc-assign-operator"); CheckFactories.registerCheck( alexfh wrote: > Check names usually

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

alexfh added a comment. Adding the original author in case he has something to say here. Comment at: clang-tidy/misc/AssignOperatorCheck.h:24 @@ +23,3 @@ +/// * Works with move-assign and assign by value. +/// * Private and deleted operators are ignored. +class

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware marked an inline comment as done. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:75 @@ +74,3 @@ +static const char *const Messages[][2] = { +{"ReturnType", "operator=() should return '%0&'"}, +{"ArgumentType", "operator=() should take

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware updated this revision to Diff 51906. baloghadamsoftware added a comment. Requested fixes done (not related to the changes). http://reviews.llvm.org/D18265 Files: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp clang-tidy/misc/AssignOperatorCheck.cpp

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

aaron.ballman added a comment. In http://reviews.llvm.org/D18265#385300, @baloghadamsoftware wrote: > Thank you for your comments, but they are not related to my changes. These > lines were present in the original file and I did not change them. Ah, good to know (hard to tell context in Phab

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added a comment. Thank you for your comments, but they are not related to my changes. These lines were present in the original file and I did not change them. http://reviews.llvm.org/D18265 ___ cfe-commits mailing list

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

aaron.ballman added a subscriber: aaron.ballman. aaron.ballman added a reviewer: aaron.ballman. Comment at: clang-tidy/misc/AssignOperatorCheck.cpp:74 @@ +73,3 @@ + } else { +std::string Name = Method->getParent()->getName(); + This can be lowered into the

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware updated this revision to Diff 51554. baloghadamsoftware added a comment. Merged into misc-assign-operator-signature and thus renamed to misc-assign-operator http://reviews.llvm.org/D18265 Files: clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

Eugene.Zelenko added a comment. As user I could tell that it's much easier to have one check which will check all nuances of particular language construction then multiple checks. http://reviews.llvm.org/D18265 ___ cfe-commits mailing list

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware added a comment. My first thought was also to extend existing checker misc-assign-operator-signature and rename it to just misc-assign-operator. However, there is little benefit doing this: the two checkers check different locations, one checks the signature while the other

Re: [PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

Eugene.Zelenko added a subscriber: Eugene.Zelenko. Eugene.Zelenko added a comment. I think will be reasonable to merge misc-assign-operator-return and misc-assign-operator-signature into one check. http://reviews.llvm.org/D18265 ___ cfe-commits

[PATCH] D18265: [clang-tidy] New: checker misc-assign-operator-return

baloghadamsoftware created this revision. baloghadamsoftware added reviewers: alexfh, hokein. baloghadamsoftware added subscribers: cfe-commits, xazax.hun. Finds return statements in assign operator bodies where the return value is different from '*this'. Only assignment operators with correct