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

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

Szelethus added a comment.

Thanks @NoQ  for taking the time to review my code!

I'm sorry that the patch became this long. It was an error on my part, and I completely get that the checker now takes an uncomfortably long time to read and understand. This is my first "bigger" project in the CSA, I learned a lot so far, and I'll definitely review how I should've handled developing and uploading it here better, so next time I will do my best to make my contributions a lot more reviewer-friendly.

> It's very unlikely that you have a single idea that takes 500 non-trivial lines of code to express.

That's true. Since this was the first time I wrote anything the StaticAnalyzer project, I often found myself almost completely rewriting everything. Looking back, I still could've uploaded the code in parts, so notes taken, will do better next time!

It'll be some time before I sort everything out you commented on, just wanted to leave this here.



================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260
+    Report->addNote(NoteBuf,
+                    PathDiagnosticLocation::create(FieldChain.getEndOfChain(),
+                                                   Context.getSourceManager()));
----------------
NoQ wrote:
> Aha, ok, got it, so you're putting the warning at the end of the constructor and squeezing all uninitialized fields into a single warning by presenting them as notes.
> 
> Because for now notes aren't supported everywhere (and aren't always supported really well), i guess it'd be necessary to at least provide an option to make note-less reports before the checker is enabled by default everywhere. In this case notes contain critical pieces of information and as such cannot be really discarded.
> 
> I'm not sure that notes are providing a better user experience here than simply saying that "field x.y->z is never initialized". With notes, the user will have to scroll around (at least in scan-build) to find which field is uninitialized.
> 
> Notes will probably also not decrease the number of reports too much because in most cases there will be just one uninitialized field. Though i agree that when there is more than one report, the user will be likely to deal with all fields at once rather than separately.
> 
> So it's a bit wonky.
> 
> We might want to enable one mode or another in the checker depending on the analyzer output flag. Probably in the driver (`RenderAnalyzerOptions()`).
> 
> It'd be a good idea to land the checker into alpha first and fix this in a follow-up patch because this review is already overweight.
> [...]i guess it'd be necessary to at least provide an option to make note-less reports before the checker is enabled by default everywhere. In this case notes contain critical pieces of information and as such cannot be really discarded.
This can already be achieved with `-analyzer-config notes-as-events=true`. There no good reason however not to make an additional flag that would emit warnings instead of notes.
> I'm not sure that notes are providing a better user experience here than simply saying that "field x.y->z is never initialized". With notes, the user will have to scroll around (at least in scan-build) to find which field is uninitialized.
This checker had a previous implementation that emitted a warning for each uninitialized field, instead of squeezing them in a single warning per constructor call. One of the major reasons behind rewriting it was that it emitted so many of them that it was difficult to differentiate which warning belonged to which constructor call. Also, in the case of the LLVM/Clang project, the warnings from this checker alone would be in the thousands.
> Notes will probably also not decrease the number of reports too much because in most cases there will be just one uninitialized field.
While this is true to some extent, it's not uncommon that a single constructor call leaves 7 uninitialized -- in the LLVM/Clang project I found one that had over 30!
Since its practically impossible to determine whether this was a performance enhancement or just poor programming, the few cases where it is intentional, an object would emit many more warnings would make  make majority of the findings (where an object truly had only 1 or 2 fields uninit) a lot harder to spot in my opinion.

In general, I think one warning per constructor call makes the best user experience, but a temporary solution should be implemented until there's better support for notes.

I agree, this should be a topic of a follow-up patch.



================
Comment at: test/Analysis/cxx-uninitialized-object.cpp:1412
+  struct RecordType;
+  // no-crash
+  RecordType *recptr; // expected-note{{uninitialized pointer 'this->recptr}}
----------------
whisperity wrote:
> What is this comment symbolising? Is this actually something the test infrastructure picks up?
`// no-crash` is something I found in many other test files, in this case it symbolizes that the point of this test isn't to check whether the checker correctly finds or ignores an uninit field, but that it doesn't crash. 


https://reviews.llvm.org/D45532