LegalizeAdulthood marked 2 inline comments as done. LegalizeAdulthood added inline comments.
================ Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:389-390 +- Test your check under both Windows and Linux environments. +- Watch out for high false positive rates; ideally there should be none, but a + small number may be tolerable. High false positive rates prevent adoption. +- Consider configurable options for your check to deal with false positives. ---------------- ymandel wrote: > aaron.ballman wrote: > > LegalizeAdulthood wrote: > > > ymandel wrote: > > > > Is it worth giving a rule-of-thumb? Personally I'd go with < 10%, all > > > > things being equal. A check for a serious bug may reasonably have a > > > > higher false positive rate, and trivial checks might not justify *any* > > > > false positives. But, if neither of these apply, I'd recommend 10% as > > > > the default. > > > I'm OK with rule-of-thumb 10% advice. > > FWIW, I think 10% is pretty arbitrary and I'd rather not see us try to nail > > it down to a concrete number. In practical terms, it really depends on the > > check. > > > > Also, clang-tidy is where we put things with a "high" false positive rate > > already, so this statement has implications on what an acceptable false > > positive rate is for Clang or the static analyzer. > > > > How about something along these lines: > > ``` > > - Watch out for high false positive rates. Ideally, a check would have no > > false positives, but given that matching against an AST is not control- or > > data flow-sensitive, a number of false positives are expected. The higher > > the false positive rate, the less likely the check will be adopted in > > practice. Mechanisms should be put in place to help the user manage false > > positives. > > - There are two primary mechanisms for managing false positives: supporting > > a code pattern which allows the programmer to silence the diagnostic in an > > ad hoc manner and check configuration options to control the behavior of > > the check. > > - Consider supporting a code pattern to allow the programmer to silence the > > diagnostic whenever such a code pattern can clearly express the > > programmer's intent. For example, allowing an explicit cast to `void` to > > silence an unused variable diagnostic. > > - Consider adding check configuration options to allow the user to opt into > > more aggressive checking behavior without burdening users for the common > > high-confidence cases. > > ``` > > (or something along those lines). The basic idea I have there is: false > > positives are expected, try to keep them to a minimum, and here are the two > > most common ways code reviewers will ask you to handle false positives when > > they're a concern. > Strongly agree. 10% has served as well in practice for the threshold at which > we disable/fix checks, but it's definitely arbitrary. I much prefer your > suggested approach. Yeah, I wasn't a fan of a magic number style piece of advice. I like the reworded suggestion better. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117939/new/ https://reviews.llvm.org/D117939 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
