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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:31
+/// every other element is a field, and the element that precedes it is the
+/// object that contains it.
+class FieldChainInfo {
----------------
As far as I understand, for every operation, the only relevant contribution of the non-last elements in the chain is the diagnostic message.
I wonder if you would build a string instead of a FieldRegion chain and only store the last FieldRegion would make this more explicit in the code. 


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+  if (isCalledByConstructor(Context))
+    return;
----------------
Szelethus wrote:
> whisperity wrote:
> > I think somewhere there should be a bit of reasoning why exactly these cases are ignored by the checker.
> At this function's declaration I have some comments describing what this does and why it does it. Did you find it insufficient?
I am a bit surprised why these errors are not deduplicated by the analyzer. Maybe it would worth some investigation?


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:272
+// If Chain's value is None, that means that this function was called to
+// determine whether a union has any initialized fields. In this case we're not
+// collecting fields, we'd only like to know  whether the value contained in
----------------
I find the documentation and the name of the function below misleading. `isNonUnionUninit` tells me this function is not supposed to be called for unions but the documentation suggests otherwise.


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:283
+  bool ContainsUninitField = false;
+  Optional<FieldChainInfo> LocalChain = Chain;
+
----------------
Why do you copy here explicitly instead of taking the `Chain` argument by value?


================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:292
+      if (LocalChain)
+        LocalChain->push_back(FR);
+      if (isNonUnionUninit(FR, LocalChain))
----------------
Don't you need to pop somewhere?


https://reviews.llvm.org/D45532