[PATCH] D16517: ClangTidy check to flag uninitialized builtin and pointer fields.

[Alexander Kornienko via cfe-commits]

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