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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Aside from the unit testing bit, I think this is fantastic! (And the unit testing bit may also be fantastic, I just think it needs more explicit discussion with a wider audience.)



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


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


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

https://reviews.llvm.org/D117939