aaron.ballman added a subscriber: aaron.ballman. aaron.ballman requested changes to this revision. aaron.ballman added a reviewer: aaron.ballman. This revision now requires changes to proceed.
================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:25 @@ -24,3 +24,3 @@ -namespace { ---------------- Please do not remove the unnamed namespace; it is preferable to using static functions. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:30 @@ +29,3 @@ + const auto *ClassDecl = dyn_cast<CXXRecordDecl>(&RecordDecl); + // Non-C++ records are always trivially constructible + if (!ClassDecl) ---------------- Missing a period at the end of the sentence. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:33 @@ +32,3 @@ + return true; + // A class is trivially constructible if it has a default constructor. + if (ClassDecl->hasUserProvidedDefaultConstructor()) ---------------- This comment doesn't match the behavior of the code below. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:104 @@ -35,3 +103,3 @@ QualType Type = F->getType(); - if (Type->isPointerType() || Type->isBuiltinType()) + if (!F->hasInClassInitializer() and isTriviallyConstructible(Context, Type)) FieldsToInit.insert(F); ---------------- Please use `&&` instead of `and`. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:152 @@ +151,3 @@ + +AST_MATCHER(CXXConstructorDecl, isUserProvided) { + return Node.isUserProvided(); ---------------- This should probably be made more generally available as an AST matcher. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:156 @@ +155,3 @@ + +AST_MATCHER(RecordDecl, isTriviallyDefaultConstructible) { + return isTriviallyConstructible(Finder->getASTContext(), Node); ---------------- I think this matcher logic should go into utils\TypeTraits.cpp as it seems more generally useful than just this check. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:160 @@ +159,3 @@ + +AST_MATCHER(CXXConstructorDecl, isDelegatingConstructor) { + return Node.isDelegatingConstructor(); ---------------- As should this. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:187 @@ -87,5 +186,3 @@ const CXXConstructorDecl &Constructor) const { - if (InitializerBefore != nullptr) { - return Lexer::getLocForEndOfToken(InitializerBefore->getRParenLoc(), 0, - Context.getSourceManager(), - Context.getLangOpts()); + assert(Where != nullptr or Placement == InitializerPlacement::New); + SourceLocation Location; ---------------- Please use `||` instead of `or`. Also, asserts should usually have `&& "Some explanatory text"` for when the assert is triggered. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:243 @@ +242,3 @@ +// initializer. +static const NamedDecl *getInitializerDecl(const CXXCtorInitializer *Init) { + if (Init->isMemberInitializer()) ---------------- Since this function is only used once, I think it should be lowered into its use as a ternary conditional. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:256 @@ +255,3 @@ + SmallVector<IntializerInsertion, 16> Insertions; + Insertions.push_back({InitializerPlacement::New, nullptr, {}}); + ---------------- Please do not use an initializer list like this, I believe it won't work in MSVC 2013. (Here and elsewhere as well.) ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:258 @@ -142,1 +257,3 @@ + + auto Decl = std::begin(OrderedDecls); for (const CXXCtorInitializer *Init : Inits) { ---------------- Please only use `auto` when the type name is spelled out as part of the initializer, or it is used as a for-range initializer. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:296 @@ +295,3 @@ + Decls.clear(); + for (const CXXBaseSpecifier &Base : ClassDecl->bases()) + Decls.emplace_back(getRecordDecl(Base.getType())); ---------------- Can use `const auto &` here instead. ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:323 @@ -171,5 +322,3 @@ void ProTypeMemberInitCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(cxxConstructorDecl(isDefinition(), isUserProvided(), - unless(isInstantiated())) - .bind("ctor"), - this); + auto IsUserProvidedNonDelegatingConstructor = + allOf(isUserProvided(), ---------------- Since this is a C++ core guideline, I think the matchers should only be registered when compiling for C++. It may make sense to use a similar check in C mode, but we can cross that bridge when we come to it (and have tests for it). ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:486 @@ +485,3 @@ + + // Do not propose fixes in macros since we cannot place them correctly. + if (Ctor->getLocStart().isMacroID()) ---------------- I think this code belongs in `fixInitializerList` then, doesn't it? ================ Comment at: clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp:497 @@ +496,3 @@ + diag(Var->getLocStart(), "uninitialized record type: %0") + << Var->getName().str(); + ---------------- You should be able to pass in `Var` directly. ================ Comment at: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp:285 @@ +284,3 @@ +union NegativeUnionInClass { + NegativeUnionInClass() {} // No message as a union can only initialize on member + int X = 0; ---------------- s/on/one ================ Comment at: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp:291 @@ +290,3 @@ +union PositiveUnion { + PositiveUnion() : X() {} // No message as a union can only initialize on member + PositiveUnion(int) {} ---------------- s/on/one ================ Comment at: test/clang-tidy/cppcoreguidelines-pro-type-member-init.cpp:334 @@ +333,2 @@ + // CHECK-FIXES: int X{}; +}; ---------------- I'd like to see how we handle the following test: ``` struct S { S(int i); }; struct T { T(float f); }; union U { S s; T t; }; ``` http://reviews.llvm.org/D18584 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits