Any good ideas on how to evaluate it, then? (in addition to/other than something like Google where we can track the diagnostic for - a few months?)
On Tue, Apr 10, 2018 at 9:34 AM John McCall <rjmcc...@gmail.com> wrote: > Yeah, -Wself-assign in general is about catching a class of live-coding > errors which is pretty unlikely to ever make it into committed code, since > the code would need to be dead/redundant for the mistake to not be noticed. > So it’s specifically a rather poor match for the static analyzer, and I > would not expect it to catch much of anything on production code. > > John. > On Tue, Apr 10, 2018 at 18:27 David Blaikie <dblai...@gmail.com> wrote: > >> 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
