On Tue, Apr 10, 2018 at 9:25 AM Roman Lebedev <lebedev...@gmail.com> wrote:
> Because *so far* it has been running on the existing code, which i guess > was already pretty warning free, and was run through the static analyzer, which obviously should catch such issues. > Existing code this has been run over isn't necessarily run against the static analyzer already. (this has been run against a subset of (maybe all of, I forget) google, which isn't static analyzer clean by any stretch (I'm not even sure if the LLVM codebase itself is static analyzer clean)). > Do you always use [clang] static analyzer, each time you build? > Great! It is indeed very good to do so. But does everyone? > > This particular issue is easily detectable without full static analysis, > and as it is seen from the differential, was very straight-forward to > implement > as a simple extension of pre-existing -Wself-assign. > > So if this is really truly unacceptable false-positive rate, ok, sure, > file a RC bug, > and i will split it so that it can be simply disabled for *tests*. > > That being said, i understand the reasons behind "keep clang diagnostics > false-positive free". I don't really want to change that. > > On Tue, Apr 10, 2018 at 7:13 PM, David Blaikie <dblai...@gmail.com> wrote: > > It's more the fact that that's /all/ the warning improvement has found so > > far. If it was 8 false positives & also found 80 actionable bugs/bad > code, > > that'd be one thing. > > > > Now, admittedly, maybe it would help find bugs that people usually catch > > with a unit test, etc but never make it to checked in code - that's > always > > harder to evaluate though Google has some infrastructure for it at least. > > > > On Tue, Apr 10, 2018 at 9:07 AM John McCall <rjmcc...@gmail.com> wrote: > >> > >> If you have a concrete suggestion of how to suppress this warning for > >> user-defined operators just in unit tests, great. I don’t think 8 > >> easily-suppressed warnings is an unacceptably large false-positive > problem, > >> though. Most warnings have similar problems, and the standard cannot > >> possibly be “must never fire on code where the programmer is actually > happy > >> with the behavior”. > >> > >> John. > >> On Tue, Apr 10, 2018 at 17:12 Nico Weber <tha...@chromium.org> wrote: > >>> > >>> "False positive" means "warning fires but didn't find anything > >>> interesting", not "warning fires while being technically correct". So > all > >>> these instances do count as false positives. > >>> > >>> clang tries super hard to make sure that every time a warning fires, it > >>> is useful for a dev to fix it. If you build with warnings enabled, that > >>> should be a rewarding experience. Often, this means dialing back a > warning > >>> to not warn in cases where it would make sense in theory when in > practice > >>> the warning doesn't find much compared to the amount of noise it > generates. > >>> This is why for example clang's -Woverloaded-virtual is usable while > gcc's > >>> isn't (or wasn't last I checked a while ago) – gcc fires always when > it's > >>> technically correct to do so, clang only when it actually matters in > >>> practice. > >>> > >>> On Tue, Apr 10, 2018 at 10:21 AM, Roman Lebedev via Phabricator via > >>> cfe-commits <cfe-commits@lists.llvm.org> wrote: > >>>> > >>>> lebedev.ri added a comment. > >>>> > >>>> In https://reviews.llvm.org/D44883#1063003, @thakis wrote: > >>>> > >>>> > This landing made our clang trunk bots do an evaluation of this > >>>> > warning :-P It fired 8 times, all false positives, and all from > unit tests > >>>> > testing that operator= works for self-assignment. > >>>> > (https://chromium-review.googlesource.com/c/chromium/src/+/1000856 > has the > >>>> > exact details) It looks like the same issue exists in LLVM itself > too, > >>>> > https://reviews.llvm.org/D45082 > >>>> > >>>> > >>>> Right, i guess i only built the chrome binary itself, not the tests, > >>>> good to know... > >>>> > >>>> > Now tests often need warning suppressions for things like this, and > >>>> > this in itself doesn't seem horrible. However, this change takes a > warning > >>>> > that was previously 100% noise-free in practice and makes it pretty > noisy – > >>>> > without a big benefit in practice. I get that it's beneficial in > theory, but > >>>> > that's true of many warnings. > >>>> > > >>>> > Based on how this warning does in practice, I think it might be > better > >>>> > for the static analyzer, which has a lower bar for false positives. > >>>> > >>>> Noisy in the sense that it correctly diagnoses a self-assignment where > >>>> one **intended** to have self-assignment. > >>>> And unsurprisingly, it happened in the unit-tests, as was expected ^ > in > >>>> previous comments. > >>>> **So far** there are no truly false-positives noise (at least no > reports > >>>> of it). > >>>> > >>>> We could help workaround that the way i initially suggested, by > keeping > >>>> this new part of the diag under it's own sub-flag, > >>>> and suggest to disable it for tests. But yes, that > >>>> > >>>> > >>>> > >>>> Repository: > >>>> rC Clang > >>>> > >>>> https://reviews.llvm.org/D44883 > >>>> > >>>> > >>>> > >>>> _______________________________________________ > >>>> cfe-commits mailing list > >>>> cfe-commits@lists.llvm.org > >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
