[PATCH] D128807: [clang][transformer] Finish plumbing `Note` all the way to the output.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

Thanks for the comments.



================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:250-251
 
+// Adds a note to the given edit or edits. If there are several edits, the note
+// is added to each one of them.
+// \code
----------------
ymandel wrote:
> li.zhe.hua wrote:
> > Are we sure that this is what someone would want? Perhaps I am not creative enough, but this seems like it could explode a large number of notes that coincide to edits, which seems like an odd thing to want.
> Agreed. I'm more inclined to only include the ASTEdit version.  I see the value of the other one, but seems easy to get wrong. Alternatively, implement it to either only add to the first edit or append/prepend a separate edit with the note.
> 
> Either way, I think the primary use should be with a standalone note generator like `ASTEdit note(TextGenerator Note)`. The idea is that the new combinator allows adding notes to a rule and we happen to store notes in ASTEdits.  Then, `withNote` is really the uncommon case where you want to supplement an existing edit or set of edits with a note. WDYT?

>  I'm more inclined to only include the ASTEdit version. I see the value of the other one, but seems easy to get wrong. Alternatively, implement it to either only add to the first edit or append/prepend a separate edit with the note.

The `EditGenerator` version was so that we can always add a note. Some generators return an `EditGenerator` instead of an `ASTEdit`. For example, `noopEdit` needs access to the match result to generate an empty range . The second version is required to work with these:

```
withNote(changeTo(anchor1, ...));   // OK, changeTo returns an `ASTEdit`.
withNote(noopEdit(...));   // not OK, noopEdit returns an `EditGenerator`, we need the second version.
```

> Either way, I think the primary use should be with a standalone note generator like ASTEdit note(TextGenerator Note). The idea is that the new combinator allows adding notes to a rule and we happen to store notes in ASTEdits. Then, withNote is really the uncommon case where you want to supplement an existing edit or set of edits with a note. WDYT?

I think you're right, we don't need the notes to be attached to a particular edit. I expect most notes to be standalone: that's actually what I need in my case, and it's also actually what the `diag` API presents to the user.

```
diag(loc1, "some warning") << FixItHint::CreateReplacement(...);
diag(loc2, "some note", clang::DiagnosticIDs::Note)
```

becomes, in transformerland:

```
makeRule(matcher, flatten(changeTo(...), note(anchor2, cat("some note"))), cat("some warning"))
```

So I think we only really need an ` ASTEdit note()` generator. I went with that solution.






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128807