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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Sep 20 08:37:04 PDT 2017


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 `cruct-expr`, like `construct-expr`?


================
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(
----------------
Wouldn't registering this matcher achieve the same goal instead of needing to re-match?


================
Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:59
+      // We want to write in the FixIt the template arguments too.
+      if (const auto *Decl = dyn_cast<ClassTemplateSpecializationDecl>(
+              Init->getBaseClass()->getAsCXXRecordDecl())) {
----------------
Please pick a name other than `Decl`, since that's a type name.


================
Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:80
+
+  auto &SM = Result.Context->getSourceManager();
+  SourceLocation StartLoc = Ctor->getLocation();
----------------
Please do not use `auto` here.


================
Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:95
+  if (Tok.is(tok::l_brace))
+    FixItMsg += ": ";
+  FixItMsg += FixItInitList;
----------------
Space before the colon?


================
Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:104
+
+  diag(Tok.getLocation(),
+       "calling an inherited constructor other than the copy constructor")
----------------
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()`.


================
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) {};
+};
----------------
Don't we want the ctor-inits to be in the same order as the bases are specified?


https://reviews.llvm.org/D33722





More information about the cfe-commits mailing list