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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits