aaron.ballman added a comment. Aside from the unit testing bit, I think this is fantastic! (And the unit testing bit may also be fantastic, I just think it needs more explicit discussion with a wider audience.)
================ 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: > > 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. Oh, interesting, thanks for pointing that out! It turns out that that `clang` and `clang-tools-extra` are different sibling directories and searching `clang` for things you expect to find in `clang-tools-extra` is not very helpful. :-D My concerns are no longer a concern 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: > > 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. > Thanks for pointing out how you're doing it in one of your checks -- I still don't think we should document that we expect people to unit test private matchers unless clang-tidy reviewers are on board with the idea in general. IMO, that's putting a burden on check authors and all the reviewers to do something that's never been suggested before (let alone documented as a best practice) -- that's worthy of open discussion instead of adding it to a patch intended to document current practices. 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
