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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

Thank you for all your comments so far! I'll probably only be able to update the diff tomorrow (with me being in the GMT + 1 timezone).

> That's a lot of code, so it'll take me some time to understand what's going on here. You might be able to help me by the large patch in smaller logical chunks.

I know, and I tried to look for ways to split this checker into smaller logical chunks, but I didn't find a satisfying solution. I'll be sure to review the comments I have in the code so it's as easy as possible to understand what I did and why I did it. Also, I'd be delighted to help any way I can to guide you through the code! :)

> That said, your checker finds non-bugs, which is always risky. Sometimes the user should be allowed to initialize the field after construction but before the first use as a potentially important performance optimization. [...] If a user wants to have his codebase "analyzer-clean", would he be able to suppress the warning without changing the behavior of the program?

I didn't implement anything explicitly to suppress warnings, so if there is nothing in the CSA by default for it, I'm afraid this isn't possible (just yet).

My checker is mainly a tool to enforce the rule that every field should be initialized in a C++ object. While I see that this approach could result  reduced performance, I think it's fine to say that those users who find this important (by 'this' I mean not initializing every field) should not enable this checker.

Nevertheless, I'd be happy to know what you think of this.

> Did you find any such intentionally uninitialized fields with your checker? Were there many of those?

I ran the checker on some projects, but it's little difficult to analyze larger ones as this checker emits very important information in notes, and those are not yet part of the plist output. But it's coming: https://reviews.llvm.org/D45407! I'll be sure to answer these questions as soon as I can.



================
Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:187
+
+  if (isCalledByConstructor(Context))
+    return;
----------------
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?


================
Comment at: test/Analysis/ctor-uninitialized-member-inheritance.cpp:24
+      : NonPolymorphicLeft1(int{}) {
+    y = 420;
+    z = 420;
----------------
whisperity wrote:
> The literal `420` is repeated //everywhere// in this file. I think this (the same value appearing over and over again) will make debugging bad if something goes haywire and one has to look at memory dumps, control-flow-graphs, etc.
Would you say that I should rather use a different number for each test case?


https://reviews.llvm.org/D45532