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

[Aaron Ballman via Phabricator via cfe-commits]

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 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.
This sounds like a deficiency with the AST that should be rectified rather than worked around. Going back to lexing the source can be very expensive (think about source files that live on a network drive, for instance) and is often tricky to get correct. For instance, it seems the lexing starts at the constructor declaration itself, so does a default argument to that copy constructor using `?:` cause issues? e.g., `S(const S&, int = 0 ? 1 : 2)`


================
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) {};
+};
----------------
szdominik wrote:
> 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.
However, that then produces additional warnings because the ctor-inits are not in the canonical order (`-Wreorder`). See http://coliru.stacked-crooked.com/a/a9d77afe87618c13


================
Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:9
+class X : public Copyable {
+	X(const X& other) : Copyable(other) {}
+	//Good code: the copy ctor call the ctor of the base class.
----------------
Please clang-format this file so it meets our usual formatting requirements.


================
Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:19
+class X2 : public Copyable2 {
+	X2(const X2& other) {};
+    // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
----------------
Spurious semicolon.


================
Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:25
+class X3 : public Copyable, public Copyable2 {
+	X3(const X3& other): Copyable(other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
----------------
Spurious semicolon (check the remainder of the file, this seems to be a common issue).


https://reviews.llvm.org/D33722