[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call
Whisperity via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 12 02:20:56 PDT 2018
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
More information about the cfe-commits
mailing list