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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
