[PATCH] D61386: [clang-tidy] Add support writing a check as a Transformer rewrite rule.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added a comment.

In D61386#1487735 <https://reviews.llvm.org/D61386#1487735>, @JonasToth wrote:

> I like the new framework!


Great!



================
Comment at: clang-tools-extra/unittests/clang-tidy/TransformerTidyTest.cpp:38
+  IfInverterTidy(StringRef Name, ClangTidyContext *Context)
+      : TransformerTidy(invertIf(), Name, Context) {}
+};
----------------
JonasToth wrote:
> If we allow to pass in a callable, that returns a `RewriteRule`we would have a more flexible framework in clang-tidy.
> Going with this for now is ok in my opinion as the only good usecase that comes to my mind would be statistics.
> 
> But If some transformation needs to remember what has been done before only a `RewriteRule` as state is not sufficient and stateful actions would require global state (MEH!).
> If we allow to pass in a callable, that returns a RewriteRule we would have a more flexible framework in clang-tidy.
> Going with this for now is ok in my opinion as the only good usecase that comes to my mind would be statistics.
I'm fine with something more general, but I don't think I follow the particulars you have in mind.  Even w/ a callable, wouldn't that only be invoked *once* by clang-tidy, when it creates the class?  What kind of statistics do you have in mind?

> 
> But If some transformation needs to remember what has been done before only a RewriteRule as state is not sufficient and stateful actions would require global state (MEH!).

Indeed. The current design does preclude a (non gross) way of collecting state over multiple invocations.  Since this is rare, I think that's fine for now.  But, our plan is to add support for a Translation Unit level API that supports things like collecting state, adding and removing header includes, inserting using decls and any other TU level operations.









Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61386