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

Richard via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 24 07:09:13 PST 2022


LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:229
+If you need to interact macros and preprocessor directives, you will want to
+override the method ``registerPPCallbacks``.  The ``add_new_check.py`` script
+does not generate an override for this method in the starting point for your
----------------
aaron.ballman wrote:
> As usual, we're not super consistent, but most of our documentation is single-spaced (can you correct this throughout your changes?).
People keep asking for this, but it doesn't matter for the final output and it isn't
specified anywhere in the style guide.


================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:233
+
+If your check applies only to specific dialect of C or C++, you will
+want to override the method ``isLanguageVersionSupported`` to reflect that.
----------------
aaron.ballman wrote:
> I made it more generic, you can use this for more than just checking languages (you can check for other language features like whether `char` is signed or unsigned, etc).
This came up in another review, but if you have a check that applies
to C++11 or later, do you have to check all the versions or can you
assume that the C++11 flag will be set when C++14 is requested
via command-line options?


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


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


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

https://reviews.llvm.org/D117939



More information about the cfe-commits mailing list