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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@NoQ Do you reckon these tests files are too long? Perhaps the one about this inheritance, that inheritance, diamond inheritance, etc. could be split into multiple files.



================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:317
 
+def CtorUninitializedMemberChecker: Checker<"CtorUninitializedMember">,
+     HelpText<"Reports uninitialized members in constructors">,
----------------
This always pokes me in the eye, but shouldn't this file be sorted alphabetically?


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:103
+  //     - pointer/reference
+  //     - fundamental object (int, double, etc.)
+  //   * the parent of each node is the object that contains it
----------------
I believe `fundamental object` should be rephrased as `of fundamental type` (as in: object that is of fundamental type), as the standard talks about //fundamental types//.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+  if (isCalledByConstructor(Context))
+    return;
----------------
I think somewhere there should be a bit of reasoning why exactly these cases are ignored by the checker.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:218
+
+  std::string WarningMsg = std::to_string(UninitFields.size()) +
+                           " uninitialized field" +
----------------
Maybe one could use a Twine or a string builder to avoid all these unneccessary string instantiations? Or `std::string::append()`?


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:316
+
+    // At this point the field is a fundamental type.
+    if (isFundamentalUninit(V)) {
----------------
(See, you use //fundamental type// here.)


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:340
+
+// TODO As of writing this checker, there is very little support for unions in
+// the CSA. This function relies on a nonexisting implementation by assuming as
----------------
Please put `:` after `TODO`.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:342
+// the CSA. This function relies on a nonexisting implementation by assuming as
+// little about it as possible.
+bool FindUninitializedFields::isUnionUninit(const TypedValueRegion *R) {
----------------
What does `it` refer to in this sentence?


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:421
+    if (T->isUnionType()) {
+      // TODO does control ever reach here?
+      if (isUnionUninit(RT)) {
----------------
Please insert a `:` after `TODO` here too.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:475
+  if (Chain.back()->getDecl()->getType()->isPointerType())
+    return OS.str().drop_back(2);
+  return OS.str().drop_back(1);
----------------
Maybe this could be made more explicit (as opposed to a comment) by using [[http://llvm.org/doxygen/classllvm_1_1StringRef.html#a3f45a78650a72626cdf9210de0c6d701|`StringRef::rtrim(StringRef)`]], like this:

    return OS.str().rtrim('.').rtrim("->");

How does this code behave if the `Chain` is empty and thus `OS` contains no string whatsoever? `drop_back` asserts if you want to drop more elements than the length of the string.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:498
+                                                    Context.getStackFrame());
+  // getting 'this'
+  SVal This = Context.getState()->getSVal(ThisLoc);
----------------
Comment isn't formatted as full sentence.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:501
+
+  // getting '*this'
+  SVal Object = Context.getState()->getSVal(This.castAs<Loc>());
----------------
Neither here.


================
Comment at: test/Analysis/ctor-uninitialized-member.cpp:683
+// Note that the rules for unions are different in C++ and C.
+// http://lists.llvm.org/pipermail/cfe-dev/242017-March/42052912.html
+
----------------
NoQ wrote:
> I managed to find the thread, but this link doesn't work for me.
Confirmed, this link is a 404.


https://reviews.llvm.org/D45532