On Tue, Apr 10, 2018 at 12: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. > Right. We used to hold ourselves to very high standards for warnings. I'd like us to keep doing that. 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