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:! 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?

cfe-commits mailing list

Reply via email to