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

Tue Jan 25 13:51:30 PST 2022

ymandel added a comment.

Overall, this looks fantastic! You may want to consider (in a separate patch) mentioning godbolt.org, which is a great UI for interacting with clang-query and the AST. Example configuration: https://godbolt.org/z/v88W8ET19

In D117939#3266234 <https://reviews.llvm.org/D117939#3266234>, @LegalizeAdulthood wrote:

> In D117939#3266228 <https://reviews.llvm.org/D117939#3266228>, @njames93 wrote:
>
>> @aaron.ballman I think this has exposed some limitations with the add-new-check script. Maybe there is merit for the script to be updated to support preprocessor callbacks and options, WDYT?
>
> It could certainly use an option to specify that your check is Transformer
> based.

Agreed. Happy to do this if there's interest. I already have a (google) internal version of this script that does exactly that. In fact, we set it as the default, and require users to explicitly opt out. It is our preference for all new clang tidy checks.

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:233-234
+
+If your check applies only under a specific set of language options, you will
+want to override the method ``isLanguageVersionSupported`` to reflect that.
+
----------------
nit: "be sure"? (just to vary the language a bit)

================
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.
----------------
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.

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:301-317
+The Transformer library allows you to write a check that transforms source code by
+expressing the transformation as a ``RewriteRule``.  The Transformer library provides
+functions for composing edits to source code to create rewrite rules.  Unless you need
+to perform low-level source location manipulation, you may want to consider writing your
+check with the Transformer library.  The `Clang Transformer Tutorial
+<https://clang.llvm.org/docs/ClangTransformerTutorial.html>`_ describes the Transformer
+library in detail.
----------------
aaron.ballman wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > I think this documentation is really good, but at the same time, I don't think we have any clang-tidy checks that make use of the transformer library currently. I don't see a reason we shouldn't document this more prominently, but I'd like to hear from @ymandel and/or @alexfh whether they think the library is ready for officially supported use within clang-tidy and whether we need any sort of broader community discussion. (I don't see any issues here, just crossing t's and dotting i's in terms of process.)
> > > > There are at least two checks that use the Transformer library currently.
> > > The only mentions of `TransformerClangTidyCheck.h` that I can find are in `ClangTransformerTutorial.rst` and `clang-formatted-files.txt`, so if there are other checks using this functionality, they're not following what's documented here.
> > `CleanupCtadCheck`, `StringFindStrContainsCheck`, and `StringviewNullptrCheck` all derive from `TransformerClangTidyCheck`.
> > 
> > The first two are in the `abseil` module and the last is in the `bugprone` module.
> Oh, interesting, thanks for pointing that out! It turns out that that `clang` and `clang-tools-extra` are different sibling directories and searching `clang` for things you expect to find in `clang-tools-extra` is not very helpful. :-D
> 
> My concerns are no longer a concern here.
My 2cents: definitely comfortable encouraging adoption. In fact, we require it on internal checks unless the user has a strong reason to opt out.

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:360-361
+
+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:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > Do we have any private matchers that are unit tested? My understanding was that the public matchers were all unit tested, but that the matchers which are local to a check are not exposed via a header file, and so they're not really unit testable.
> > > > I just did this for my switch/case/label update to simplify boolean expressions.
> > > > You do have to expose them via a header file, which isn't a big deal.
> > > > You do have to expose them via a header file, which isn't a big deal.
> > > 
> > > Then they become part of the public interface for the check and anything which includes the header file has to do heavy template instantiation work that contributes more symbols to the built library. In general, we don't expect private matchers to be unit tested -- we expect the tests for the check to exercise the matcher appropriately.
> > Look at my review to see how I handled it; the matchers are in a seperate
> > header file, in my case `SimplifyBooleanExprMatchers.h` and aren't exposed
> > to consumers of the check.  The matchers header is only included by the
> > implementation and the matcher tests.
> > 
> Thanks for pointing out how you're doing it in one of your checks -- I still don't think we should document that we expect people to unit test private matchers unless clang-tidy reviewers are on board with the idea in general. IMO, that's putting a burden on check authors and all the reviewers to do something that's never been suggested before (let alone documented as a best practice) -- that's worthy of open discussion instead of adding it to a patch intended to document current practices.
Agreed on this point -- that we should push this off to discussion on a separate patch. I'm definitely fine with pointing out that unit testing is possible, since that may not be obvious. But, I'd have to be convinced that we want to insist on it.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117939/new/

https://reviews.llvm.org/D117939