ymandel 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 ---------------- aaron.ballman wrote: > 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. +1 ================ 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. ---------------- 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. 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
