aaron.ballman added a comment. In D117939#3266228 <https://reviews.llvm.org/D117939#3266228>, @njames93 wrote:
> @aaron.ballman I think this has exposed some limitations with the > add-new-check script. Maybe there is merit for the script to be updated to > support preprocessor callbacks and options, WDYT? I wouldn't be opposed to it. I think it currently serves the majority of the needs (there are far less preprocessor checks than there are AST checks), but making it more full-featured would be a win for some folks. ================ 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 ---------------- LegalizeAdulthood wrote: > 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. We're consistently inconsistent, but the reason why I think we tend to prefer single space is because it's less bytes for everyone to download (as you say, single vs double space doesn't matter for the generated output) and generally we're all reading this on computer screens rather than in print (where double spacing actually helped readability). ================ 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. ---------------- LegalizeAdulthood wrote: > 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? Later modes will also set earlier modes. e.g., passing `-std=c++20` on the command line will set `CPlusPlus`, `CPlusPlus11`, `CPlusPlus14`, `CPlusPlus17`, and `CPlusPlus20` in `LangOptions`. ================ 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: > > 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. ================ 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: > > 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. 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
