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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
