[PATCH] D117939: [clang-tidy] Add more documentation about check development

[Richard via Phabricator via cfe-commits]

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