aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:362 +Private custom matchers are a good example of auxiliary support code for your check +that is best tested with a unit test. It will be easier to test your matchers or +other support classes by writing a unit test than by writing a ``FileCheck`` integration ---------------- LegalizeAdulthood wrote: > If I change the wording from "is best tested with a unit test" to "can be > tested with a unit test", > would that alleviate the concern? I want to encourage appropriate testing > and unit testing > complex helper code is better than integration testing helper code. > > I find it easier to have confidence in private matchers if they are unit > tested and I've recently > had a few situations where I had to write relatively complex helper functions > to analyze raw > text that I felt would have been better tested with a unit test than an > integration test. > If I change the wording from "is best tested with a unit test" to "can be > tested with a unit test", would that alleviate the concern? I largely addresses mine -- saying it can be done is great, saying it should be done is what gave me pause. ================ 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. ---------------- 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. 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
