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

Mon Jan 24 06:21:09 PST 2022

aaron.ballman added a comment.

Thank you so much for working on this documentation, I really like the direction it's going!

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:78
+build :program:`clang-tidy`.
+Since your new check will have associated documentation, you will also want to install
+`Sphinx <https://www.sphinx-doc.org/en/master/>`_ and enable it in the CMake configuration.
----------------

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:228

+If you need to interact macros and preprocessor directives, you will want to
+override the method ``registerPPCallbacks``.  The ``add_new_check.py`` script
----------------

================
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
----------------
As usual, we're not super consistent, but most of our documentation is single-spaced (can you correct this throughout your changes?).

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

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:262
+Clang is implemented on top of LLVM and introduces its own set of classes that you
+will interact with while writing your check.  The most important of these are the
+classes relating the source code locations, source files, ranges of source locations
----------------
I'd argue the most important thing you interact with from Clang are the AST nodes. Maybe instead of "most important", we can be vague and just say "Some commonly used features are" or something like that?

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

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:339-340
+matching expressions to simplify your matcher.  Just like breaking up a huge function
+into smaller chunks with intention revealing names can help you understand a complex
+algorithm, breaking up a matcher into smaller matchers with intention revealing names
+can help you understand a complicated matcher.  Once you have a working matcher, the
----------------

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:343
+C++ API will be virtually identical to your interactively constructed matcher.  You can
+use local variables to preserve your intention revealing names that you applied to
+nested matchers.
----------------

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

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:370
+Once you've covered your check with the basic "happy path" scenarios, you'll want to
+torture your check with some crazy C++ in order to ensure your check is robust.  Running
+your check on a large code base, such as Clang/LLVM, is a good way to catch things you
----------------
Reworded to avoid a loaded term and not make it sound C++ specific; needs re-flowing to 80-col limits

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:372-373
+your check on a large code base, such as Clang/LLVM, is a good way to catch things you
+forgot to account for in your matchers.  However, the LLVM code base is "reasonable" and
+doesn't contain weird template or macro oriented code.
+
----------------

================
Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:383
+- Define template classes that contain code matched by your check.
+- Define template specializations that contain code matched by your check.
+
----------------
Another one that catches people out is testing on Windows vs non-Windows targets (as targets which are compatible with MSVC default to a different template instantiation behavior).

One key thing that's not discussed explicitly are false positive rates. Clang-tidy can have significantly higher false positive rates than diagnostics in Clang or the static analyzer, but we still care about not being overly chatty. But "overly chatty" depends on the check -- if the check is for a coding standard and the coding standard says "no calls to 'foo()'", then it's not chatty to diagnose every call to "foo()". But if the check is not following a coding standard, but is instead trying to help someone modernize their code, find bugs, or make it more readable (as examples), then perhaps diagnosing every call to "foo()" will be too chatty. So we ask people to test their code against real world code bases to try to gauge what the false positive and true positive rates are for the check, just to make sure they seem reasonable.

Another thing we may want to mention is that checks can be configured. This helps not only with exposing different functionality for the check, but also can help to control perceived false positives for some use cases.

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

https://reviews.llvm.org/D117939