LegalizeAdulthood marked 16 inline comments as done. LegalizeAdulthood added inline comments.
================ 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: > 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. ================ 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: > 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. 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
