[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 Jun 14 16:25:45 PDT 2017


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(
+                  isBaseInitializer(),
----------------
It seems like you can hoist out from the `cxxCtorInitializer()` onward and only write this code once rather than twice.


================
Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:48
+
+  std::string FixItInitList = "";
+  for (const auto &Match : Matches) {
----------------
No need for the initializer.


================
Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:53
+    // Valid when the initializer is written manually (not generated).
+    if ((Init->getSourceRange()).isValid()) {
+      const auto *CExpr = Match.getNodeAs<CXXConstructExpr>("cruct-expr");
----------------
Spurious parens.


================
Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:55
+      const auto *CExpr = Match.getNodeAs<CXXConstructExpr>("cruct-expr");
+      diag(CExpr->getLocEnd(), "Missing parameter in the base initializer!")
+          << FixItHint::CreateInsertion(
----------------
Diagnostics are not full sentences, should this should use a lowercase m and drop the exclamation mark. Also, it should be "argument" rather than parameter.

I think it might be more clearly rewritten as: `"calling an inherited constructor other than the copy constructor"`. Also, it would be helpful to point to which constructor *is* being called as a note.


================
Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:63
+      // We want to write in the FixIt the template arguments too.
+      if (auto *Decl = dyn_cast<ClassTemplateSpecializationDecl>(
+              Init->getBaseClass()->getAsCXXRecordDecl())) {
----------------
`const auto *`


================
Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:67
+
+        const auto Args = Decl->getTemplateArgs().asArray();
+        for (const auto &Arg : Args)
----------------
Do not use `auto` here.


================
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);
----------------
This doesn't balance tokens. What about:
```
struct S {
 /* ... */
};

struct T : S {
  T(const T &O) : S((1 + 2), O) {}
};
```


================
Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:96
+
+  std::string FixItMsg = "";
+  // There is not initialization list in this constructor.
----------------
No initializer needed.


================
Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:108-109
+
+  diag(Tok.getLocation(), "Copy constructor should call the copy "
+                          "constructor of all copyable base class!")
+      << FixItHint::CreateInsertion(Tok.getLocation(), FixItMsg);
----------------
Similar comments about the diagnostic as above. I think the two diagnostics should really be the same string. At the end of the day, the check is to ensure that a derived copy constructor calls an inherited copy constructor. Whether the copy constructor calls an inherited default constructor, or some other kind of constructor, is largely immaterial because they're both wrong in the same way. I'd recommend using the diagnostic text from my comment above for both.


https://reviews.llvm.org/D33722





More information about the cfe-commits mailing list