[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[George Karpenkov via Phabricator via cfe-commits]

george.karpenkov added a comment.

The code looks good apart from a few minor nits. I think I would prefer a new category created for this checker instead of using `alpha`; let's see what @NoQ has to say.



================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:305
+     HelpText<"Reports uninitialized members in constructors">,
+     DescFile<"CtorUninitializedMemberChecker.cpp">;
+
----------------
`alpha` might not be a right place for this checker.
The  meaning of `alpha` seems to be "this checker is currently too immature to be used by default because it generates too many FPs".

This is not the case for the uninitialized-fields checker, as it finds non-bugs by design, and each project might want to decide whether using such checker is a right fit.
I would suggest introducing a new category here, e.g. `bugprone` (other suggestions: `lint`?)
@NoQ any objections?


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:162
+/// We can't know the type of the region that a void pointer points to, so FD
+/// can't be analyzed if this function return true for it.
+bool isVoidPointer(const FieldDecl *FD);
----------------
"returns"


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:212
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+                           " uninitialized field" +
----------------
nitpicking: llvm::Twine is normally used for such constructs


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:448
+bool isVoidPointer(const FieldDecl *FD) {
+  QualType T = FD->getType();
+
----------------
Might be a naive question, but what about a chain intermixing references and pointers?
Wouldn't it be simpler to write

```
while (T)
  if (T->isVoidPointerType())
    return true;
  T = T->getPointeeType();
```

?


================
Comment at: test/Analysis/ctor-uninitialized-member.cpp:817
+    UnionWithRecord(){};
+  } u; // xpected-note{{uninitialized field 'this->u'}}
+
----------------
What happened to `e`? Here and in all notes below.


https://reviews.llvm.org/D45532