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?  | |
| 129 | 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.  | |
| 129 | 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 | ||
|---|---|---|
| 110–124 | 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.  | |
| 129 | Accepting the std:function directly should simplify this and avoid the need for the Result<void> specialization.  | |
| clang/include/clang/Tooling/Transformer/Transformer.h | ||
|---|---|---|
| 129 | 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 | ||
|---|---|---|
| 110–124 | ||
Update comments
| clang/include/clang/Tooling/Transformer/Transformer.h | ||
|---|---|---|
| 110–124 | 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.)