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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D128807#3627625 <https://reviews.llvm.org/D128807#3627625>, @courbet wrote:

> In D128807#3625676 <https://reviews.llvm.org/D128807#3625676>, @ymandel wrote:
>
>>> For my particular case, I just need multiple notes per rule. I don't need them to be associated to a particular edit (and in that very particular case, I don't even need a source location).
>>
>> I'm not sure I understand: you need this additional note, but it doesn't need to be tied to a source location, so, why can't you concatenate it to the main note?
>
> For my specific check the notes carry structure beyond what's typically in a warning, so I need the information to be separate.

Based on our conversation offline, I'm inclined towards the change with the note below about API.  Thanks!



================
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
----------------
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?


================
Comment at: clang/include/clang/Tooling/Transformer/RewriteRule.h:255-256
+// \endcode
+EditGenerator withNote(EditGenerator Generator, TextGenerator Note);
+ASTEdit withNote(ASTEdit Edit, TextGenerator Note);
+
----------------



================
Comment at: clang/lib/Tooling/Transformer/RewriteRule.cpp:56
     T.Range = *EditRange;
-    T.Replacement = std::move(*Replacement);
-    T.Metadata = std::move(*Metadata);
+    if (E.Replacement) {
+      auto Replacement = E.Replacement->eval(Result);
----------------
courbet wrote:
> ymandel wrote:
> > Can this ever be null? I assume the intent was "no" (in which case I'd recommend an assert instead), but I'm open to the idea that it's valid, just want to understand better.
> Given that we provide an `EditGenerator edit(ASTEdit)`, we can't ever be sure that the user won't give us an empty replacement.
> 
> I've split this off to a separate patch here with a test to show the issue: https://reviews.llvm.org/D128887
Agreed. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128807