[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

This revision was automatically updated to reflect the committed changes. Closed by commit rL318522: [clang-tidy] Add a check for undelegated copy of base classes (authored by xazax). Changed prior to commit: https://reviews.llvm.org/D33722?vs=121886&id=123319#toc Repository: rL LLVM https:

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from two minor nits, the check LGTM. Whether we put it in misc or bugprone can be answered by @alexfh or by your best judgement. Comment at: clang-tidy/m

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman added inline comments. Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:69 + (Ctor->getAccess() == AS_private || Ctor->isDeleted())) { +NonCopyableBase = true; +break; xazax.hun wrote: > aaron.ballman wrote: > > What

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun added a comment. In https://reviews.llvm.org/D33722#916990, @aaron.ballman wrote: > In https://reviews.llvm.org/D33722#916540, @xazax.hun wrote: > > > Also, bugprone might be a better module to put this? > > > I don't have strong opinions on misc vs bugprone (they're both effectively >

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun updated this revision to Diff 121886. xazax.hun marked 2 inline comments as done. xazax.hun added a comment. - Fix doc comments that I overlooked earlier https://reviews.llvm.org/D33722 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/CopyConstructorInitCheck.cpp clang-tidy

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun updated this revision to Diff 121885. xazax.hun marked 3 inline comments as done. xazax.hun added a comment. - Fix review comments https://reviews.llvm.org/D33722 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/CopyConstructorInitCheck.cpp clang-tidy/misc/CopyConstructorIn

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman added a comment. In https://reviews.llvm.org/D33722#916540, @xazax.hun wrote: > Also, bugprone might be a better module to put this? I don't have strong opinions on misc vs bugprone (they're both effectively catch-alls for tidy checks, as best I can tell). https://reviews.llvm.

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman added a comment. In https://reviews.llvm.org/D33722#916539, @xazax.hun wrote: > In https://reviews.llvm.org/D33722#915440, @aaron.ballman wrote: > > > I disagree. We should not be changing code to be incorrect, requiring the > > user to find that next incorrectness only through an

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun added a comment. Also, bugprone might be a better module to put this? https://reviews.llvm.org/D33722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun added a comment. In https://reviews.llvm.org/D33722#915440, @aaron.ballman wrote: > In the other cases, it is not clear that the re-lexed information should be > carried in the AST. In this case, I think it's pretty clear that the AST > should carry this information. Further, I don't

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun updated this revision to Diff 121698. xazax.hun added a comment. - Do not warn for NonCopyable bases - Remove lexing https://reviews.llvm.org/D33722 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/CopyConstructorInitCheck.cpp clang-tidy/misc/CopyConstructorInitCheck.h cl

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman added a comment. In https://reviews.llvm.org/D33722#915134, @xazax.hun wrote: > Two problems are not resolved. One is Aaron prefers to query some infor from > the AST instead of relexing. The second is providing base initializers in the > wrong order. > I think there are other ch

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun marked 5 inline comments as done. xazax.hun added a comment. Two problems are not resolved. One is Aaron prefers to query some infor from the AST instead of relexing. The second is providing base initializers in the wrong order. I think there are other checks that do relexing in some c

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun updated this revision to Diff 121477. xazax.hun added a comment. - Dominic said he no longer have time to continue with this patch, so I commandeered this revision - Skip template instantiations - Do not attempt fix macro expansions - Do not attempt fix type aliases and typedef types -

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman added inline comments. Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:104 + + diag(Tok.getLocation(), + "calling an inherited constructor other than the copy constructor") szdominik wrote: > aaron.ballman wrote: > > Insteaad of havi

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik added inline comments. Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:37 + + // We match here because we want one warning (and FixIt) for every ctor. + const auto Matches = match( aaron.ballman wrote: > Wouldn't registering this matcher achi

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik updated this revision to Diff 118617. szdominik marked 4 inline comments as done. szdominik added a comment. Small changes after aaron.ballman's comments. https://reviews.llvm.org/D33722 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/CopyConstructorInitCheck.cpp clang-tid

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

klimek added a comment. The run on llvm indicates that we don't want this to trigger if the base class doesn't have anything to copy (that is, no fields & a defaulted copy-ctor, or an empty copy-ctor). https://reviews.llvm.org/D33722 ___ cfe-commi

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman added inline comments. Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:24 +withInitializer(cxxConstructExpr(unless(hasDescendant(implicitCastExpr( +.bind("cruct-expr"))); + You pick a more readable name than

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik added a comment. There wasn't any update on this check lately - can I help to make it better? https://reviews.llvm.org/D33722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commit

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik updated this revision to Diff 109564. szdominik marked 2 inline comments as done. szdominik added a comment. Herald added a subscriber: JDevlieghere. Fixed check-fixes lines in test cases. Updated matcher definition. https://reviews.llvm.org/D33722 Files: clang-tidy/misc/CMakeLists.

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

xazax.hun added inline comments. Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:22 +AST_MATCHER(CXXCtorInitializer, ctorInit) { + return cxxCtorInitializer( + isBaseInitializer(), Wouldn't something like: ``` static auto ctorInit = cxxCtor

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik updated this revision to Diff 103385. szdominik marked 4 inline comments as done. szdominik added a comment. Updated loop for searching the beginning of the initlist. https://reviews.llvm.org/D33722 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/CopyConstructorInitCheck.cpp

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik added inline comments. Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:92 + // Loop until the beginning of the initialization list. + while (!Tok.is(tok::r_paren)) +Lex.LexFromRawLexer(Tok); aaron.ballman wrote: > szdominik wrote

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman added inline comments. Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:92 + // Loop until the beginning of the initialization list. + while (!Tok.is(tok::r_paren)) +Lex.LexFromRawLexer(Tok); szdominik wrote: > aaron.ballman w

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik added inline comments. Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:92 + // Loop until the beginning of the initialization list. + while (!Tok.is(tok::r_paren)) +Lex.LexFromRawLexer(Tok); aaron.ballman wrote: > This doesn't ba

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik updated this revision to Diff 102807. szdominik marked 9 inline comments as done. szdominik added a comment. Rename check. Hoisted matcher, changed warning message & nits. https://reviews.llvm.org/D33722 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/CopyConstructorInitChec

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

malcolm.parsons added a comment. This check is similar to `misc-move-constructor-init`; could it have a similar name please. https://reviews.llvm.org/D33722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman requested changes to this revision. aaron.ballman added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:41 + forEachConstructorInitializer( + cxxCtorInitializer(

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik marked 2 inline comments as done. szdominik added a comment. Warnings of the check's run on llvm/clang codebase. F3426875: undelegated-copy-of-base-classes-clangrun.txt https://reviews.llvm.org/D33722

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

alexfh added a comment. I would be interested in seeing the results of this check's run on LLVM+Clang code. https://reviews.llvm.org/D33722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:83 + + Finds copy constructors where the ctor don't call the constructor of the base class. + I think will be good idea to replace ctor with constructor. Or re-phrase sentence to avoid rep

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik updated this revision to Diff 100892. szdominik added a comment. Update with fixed docs. https://reviews.llvm.org/D33722 Files: clang-tidy/misc/CMakeLists.txt clang-tidy/misc/MiscTidyModule.cpp clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp clang-tidy/misc/UndelegatedCo

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

Eugene.Zelenko added a comment. Please mention this check in docs/ReleaseNotes.rst (in alphabetical order). Comment at: docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst:29 + +The checker suggests a FixItHint in every scenario including multiple +missing initial

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

szdominik created this revision. Herald added a subscriber: mgorny. Finds copy constructors where the constructor don't call the constructor of the base class. class X : public Copyable { X(const X& other) {}; // Copyable(other) is missing }; Also finds copy constructors where the bas