NoQ added inline comments.
================ Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:258-260 + Report->addNote(NoteBuf, + PathDiagnosticLocation::create(FieldChain.getEndOfChain(), + Context.getSourceManager())); ---------------- NoQ wrote: > Szelethus wrote: > > 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. > > > > This can already be achieved with `-analyzer-config notes-as-events=true`. > > Yep. But the resulting user experience seems pretty bad to me. It was a good > quick proof-of-concept trick to test things, but the fact that notes aren't > path pieces is way too apparent in case of this checker. So i guess this flag > was a bad idea. > > > Also, in the case of the LLVM/Clang project, the warnings from this checker > > alone would be in the thousands. > > This makes me a bit worried, i wonder what's the reason why does the checker > shout so loudly on a codebase that doesn't seem to have actual uninitialized > value bugs all over the place. > > Are any of these duplicates found in the same constructor for the same field > in different translation units? Suppose we discard the duplicates (if any) > and warn exactly once (across the whole LLVM) for every uninitialized field > (which is probably more than once per constructor). Then I wonder: > 1. How many uninitialize fields would we be able to find this way? > 2. How many of such fields were intentionally left uninitialized? > 3. How many were found by deep inspection of the heap through field chains > involving pointer dereference "links"? (when i'm asking this sort of stuff, i don't mean you should look through thousands of positives, a uniform random sample of ~50 should be just fine) (but we really do need this info before enabling stuff by default) https://reviews.llvm.org/D45532 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
