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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 16 inline comments as done.
LegalizeAdulthood added inline comments.


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


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



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

https://reviews.llvm.org/D117939