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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
