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

[Dominik Szabó via Phabricator via cfe-commits]

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 achieve the same goal instead of needing to re-match?
I wanted to create //only one// FixIt to every ctor - if I move the forEachCtorInitializer to the register part, it would warn us twice.


================
Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:104
+
+  diag(Tok.getLocation(),
+       "calling an inherited constructor other than the copy constructor")
----------------
aaron.ballman wrote:
> Insteaad of having to re-lex the physical source, can the AST should be modified to carry the information you need if it doesn't already have it? For instance, you can tell there is not initializer list by looking at `CXXConstructorDecl::getNumCtorInitializers()`.
The getNumCtorInitializers method counts the generated initializers as well, not just the manually written ones.
My basic problem was that the AST's methods can't really distinguish the generated and the manually written initializers, so it's quite complicated to find the locations what we need. I found easier & more understandable if I start reading the tokens.


================
Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:27
+	// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X3(const X3& other): Copyable2(other), Copyable(other) {};
+};
----------------
aaron.ballman wrote:
> Don't we want the ctor-inits to be in the same order as the bases are specified?
I think it's more readable if we put the FixIts to the beginning of the expression - it's easier to check that everyting is correct & it's more obvious what the changes are.


https://reviews.llvm.org/D33722