NoQ added a comment.

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.

The idea looks reasonable. We're having specific warnings for, eg., assigning 
an uninitialized value, which fire much more often now that we have proper 
constructor support (eg. they'll fire in the copy constructor when the value 
was not initialized in the "actual" constructor). While some bugs can be found 
this way, i agree that it's not the best way to find them.

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. Did you find 
any such intentionally uninitialized fields with your checker? Were there many 
of those? 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?

If union support is causing problems and you have a lot of workaround in the 
checker for these problems, i'll definitely suggest removing these workarounds 
from the initial commit and replacing them with FIXME comments (even if it 
assumes completely suppressing all warnings on classes that mention unions 
anywhere in the chain) because i'd much rather move towards better modeling in 
RegionStore than having checkers work around that. You might also be interested 

> In most cases, all fields of a union is regarded as unknown. I checked these 
> cases by regarding unknown fields as uninitialized.

The whole point of making them unknown, as far as i understand, was to suppress 
uninitialized field warnings. So i feel what we're doing here is a bit upside 

Comment at: test/Analysis/ctor-uninitialized-member.cpp:683
+// Note that the rules for unions are different in C++ and C.
I managed to find the thread, but this link doesn't work for me.

Comment at: test/Analysis/ctor-uninitialized-member.cpp:869
+void f44() {
+  ContainsUnionWithSimpleUnionTest2(); // xpected-warning{{1 uninitialized 
Hmm, shouldn't it say "expected"? Do these tests actually work?

cfe-commits mailing list

Reply via email to