alexfh added a comment. In http://reviews.llvm.org/D16517#336157, @flx wrote:
> In http://reviews.llvm.org/D16517#336148, @alexfh wrote: > > > A high-level comment: I think, this comment > > <http://reviews.llvm.org/D10553#267535> still applies. I'm also slightly > > concerned about having this check in misc-, since the check isn't > > universally applicable (e.g. based on a couple of discussions, I guess LLVM > > community would likely not welcome this rule), but I'd like to leave misc- > > enabled by default. So moving the check to cppcoreguidelines- makes sense > > to me. > > > > More substantial comments later. > > > Sounds good to me. If I'm reading the original comment correctly what's > missing would be this part: > > "Issue a diagnostic when constructing an object of a trivially constructible > type without () or {} to initialize its members. To fix: Add () or {}. " Yes. I think, we might want to either add a separate check for this or make this part configurable. Adding a FIXME for now would be enough. > Please let me know if that changes the name of the check and whether the next > step should be moving the check or we'll do that after a detailed review of > the specifics. I'd call the check `cppcoreguidelines-pro-type-memberinit` to be consistent with the anchor name of this rule used in the CppCoreGuidelines document (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Pro-type-memberinit). These anchor names are usually short enough, but still quite descriptive, so it makes sense to use them when there are no significantly better alternatives. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:29 @@ +28,3 @@ + const Stmt &Stmt, ASTContext &Context, + std::unordered_set<const FieldDecl *> &FieldDecls) { + auto Matches = ---------------- Once you switch to `SmallPtrSet`, this should be changed to `SmallPtrSetImpl&`. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:104 @@ +103,3 @@ + } + Code = Stream.str(); + // The initializer list is created, replace leading comma with colon. ---------------- That's a self-assignment, basically. Just use `Stream.flush()` or put the `Stream` definition into a nested scope. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:106 @@ +105,3 @@ + // The initializer list is created, replace leading comma with colon. + if (InitializerBefore == nullptr && InitializerAfter == nullptr) { + Code[1] = ':'; ---------------- nit: No braces around single-line `if/for` bodies. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:123 @@ +122,3 @@ + } + std::vector<FieldsInsertion> OrderedFields; + OrderedFields.push_back({LastWrittenNonMemberInit, nullptr, {}}); ---------------- `SmallVector`? ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:124 @@ +123,3 @@ + std::vector<FieldsInsertion> OrderedFields; + OrderedFields.push_back({LastWrittenNonMemberInit, nullptr, {}}); + ---------------- `.emplace_back(...)`? ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:163 @@ +162,3 @@ + const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor"); + if (!Ctor->isUserProvided()) + return; ---------------- I'd make a local matcher for this and push this check to the matcher. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:169 @@ +168,3 @@ + + std::unordered_set<const FieldDecl *> FieldsToInit; + std::copy_if(MemberFields.begin(), MemberFields.end(), ---------------- This should probably use SmallPtrSet. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:170 @@ +169,3 @@ + std::unordered_set<const FieldDecl *> FieldsToInit; + std::copy_if(MemberFields.begin(), MemberFields.end(), + std::inserter(FieldsToInit, FieldsToInit.end()), ---------------- Consider this as a simpler alternative with fewer dependencies: for (const auto &Field : ParentClass->fields()) { if (fieldRequiresInit(Field)) FieldsToInit.insert(Field); } Also, might be reasonable to expand the `fieldRequiresInit` function: for (const auto &Field : ParentClass->fields()) { if (Field->getType()->isPointerType() || Field->getType()->isBuiltinType()) FieldsToInit.insert(Field); } ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:177 @@ +176,3 @@ + for (CXXCtorInitializer *Init : Ctor->inits()) { + if (Init->isDelegatingInitializer()) + return; ---------------- I think, this is superfluous. `isMemberInitializer()` should be enough. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:181 @@ +180,3 @@ + continue; + const FieldDecl *MemberField = Init->getMember(); + FieldsToInit.erase(MemberField); ---------------- The variable does not make code cleaner, please remove it. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:199 @@ +198,3 @@ + + for (const auto *Field : FieldsToInit) + Builder << FixItHint::CreateInsertion( ---------------- nit: We don't use braces only for single-line `if/for` bodies. Same below. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:207 @@ +206,3 @@ + + DiagnosticBuilder Builder = + diag(Ctor->getLocStart(), ---------------- s/Builder/Diag/, which is more common in this code. ================ Comment at: clang-tidy/misc/UninitializedFieldCheck.cpp:208 @@ +207,3 @@ + DiagnosticBuilder Builder = + diag(Ctor->getLocStart(), + "constructor does not initialize these built-in/pointer fields: %0") ---------------- No need to duplicate this code. Only the insertion of fixes should be conditional. ================ Comment at: test/clang-tidy/misc-uninitialized-field.cpp:1 @@ +1,2 @@ +// RUN: %check_clang_tidy %s misc-uninitialized-field %t + ---------------- As usual: tests with templates and macros, please ;) ================ Comment at: test/clang-tidy/misc-uninitialized-field.cpp:6 @@ +5,3 @@ + // CHECK-FIXES: int F{}; + PositiveFieldBeforeConstructor() {} + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: constructor does not initialize these built-in/pointer fields: F ---------------- Also check that the constructor is left intact? http://reviews.llvm.org/D16517 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits