Change RewriteRule from holding an Explanation to being able to generate
arbitrary metadata. Where TransformerClangTidyCheck was interested in a string
description for the diagnostic, other tools may be interested in richer metadata
at a higher level of abstraction than at the edit level (which is currently
available as ASTEdit::Metadata).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/Tooling/Transformer/RewriteRule.h | ||
---|---|---|
70 | Placeholder name; suggestions welcome. |
High level question: why use Any rather than templating?
clang/include/clang/Tooling/Transformer/RewriteRule.h | ||
---|---|---|
70 | hmm. How about MetadataGenerator? Less general, but to the point. |
(Aside: Not sure how to "respond" to comments... just quote in a new comment?)
So, I had a version of this, but I ended up bifurcating the type into RewriteRule (with no metadata) and RewriteRuleWith<T> (with T metadata). I wasn't sure if the cost of having two types here was worth it. For interfaces that didn't care about the metadata generation, there wasn't an easy way to accept a RewriteRule-like parameter.
Finally, there was a question of migration. All uses of RewriteRule with TransformerClangTidyCheck would need to switch over to RewriteRuleWith<std::string>, and I wasn't sure how pervasive that might be. I'm not set on Any, but splitting the type does seem rather costly.
Looks really good!
clang/include/clang/Tooling/Transformer/RewriteRule.h | ||
---|---|---|
64–66 | (and move down to after Generator def.) | |
286–291 | Please comment. | |
329 | Maybe add an adaptor for edit generator like we do in Stencil::cat. That will allow us to collapse the 6 overloads (2 * 3) into 2 (for makerule) + 3 (for the adaptor). Only saves 1 but I think it will clearer and more flexible for the future. | |
526–527 | please update | |
clang/include/clang/Tooling/Transformer/Transformer.h | ||
22 | in RewriteRule.h, we use "detail". Maybe do the same here? | |
48 | I'm afraid the use of the name Consumer (here and below) might be confused with the Consumer argument. What about just Impl or something similar? | |
126 | why won't we have the same SFINAE here to ensure ConsumerFn is invocable? |
Fixes from comments
clang/include/clang/Tooling/Transformer/RewriteRule.h | ||
---|---|---|
329 | So, I ended up needing to add an overload for std::initializer_list as well, as it was somewhat common for people to do something like makeRule(matchers(), {doThing1(), doThing2()}) which would have gone to the llvm::SmallVector<ASTEdit, 1> overload. | |
clang/include/clang/Tooling/Transformer/Transformer.h | ||
48 | I went with NoMetadataImpl (to be more descriptive than "plain") and WithMetadataImpl. | |
126 | I can't figure out how to implement the SFINAE without instantiating Result<void> (which is invalid because T Metadata is illegal then). My current attempt is template < typename T, typename ConsumerFn, std::enable_if_t< std::conjunction< std::negation<std::is_same<T, void>>, llvm::is_invocable<ConsumerFn, llvm::Expected<Result<T>>>>::value, int> = 0> explicit Transformer(transformer::RewriteRuleWith<T> Rule, ConsumerFn Consumer); I suppose I could provide a specialization of Result<void> that is valid? I guess I'll go with that, and just document why it exists? |
clang/include/clang/Tooling/Transformer/Transformer.h | ||
---|---|---|
107–121 | Given that we're simply passing this off to the NoMetadataImpl which accepts a std::function, I don't see any reason to accept a generic parameter via sfinae. We could just accept the std::function and move it. Then the implicit conversion from whatever is passed in to std::function will do the heavy sfinae lifting. | |
126 | Accepting the std:function directly should simplify this and avoid the need for the Result<void> specialization. |
clang/include/clang/Tooling/Transformer/Transformer.h | ||
---|---|---|
126 | Seems fine, but I'll admit to being out of my depth. Seemed nice to have, but not necessary. |
clang/include/clang/Tooling/Transformer/Transformer.h | ||
---|---|---|
107–121 |
Update comments
clang/include/clang/Tooling/Transformer/Transformer.h | ||
---|---|---|
107–121 | Nevermind! I just tried and it works. I suspect this is because the non-template constructor and the template constructor are not ambiguous, as the non-template is tried first, and if it doesn't match, it goes to the template. |
(and move down to after Generator def.)