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

Thu Jun 30 09:02:34 PDT 2022

li.zhe.hua added a comment.

In D128807#3620573 <https://reviews.llvm.org/D128807#3620573>, @ymandel wrote:

> Eric -- what do you think? You've thought a lot more about metadata recently.

(Apologies for the delay. Still struggling to figure out a good workflow for reviewing patches...)

Can you say more about what your intended use of the note is? Perhaps that can motivate some of the discussion.

>From what I can see, here's my thoughts:

- A note being a part of an edit seems weird at best. An `ASTEdit` and `Edit` are fragments of a greater, logical change. That a note should semantically be associated with the insertion of an open paren "`(`" but not the close paren "`)`" seems odd to me.
- The note takes the location of the edit it is attached to. Perhaps that could be of some convenience when those coincide, but I don't believe that should necessarily be the case. I'm imagining notes could be used to point out //other// parts of the source that are interesting.

I don't have a lot of experience with how notes appear when surfaced, but I suspect that this interface might encourage callers to tack notes on without putting a lot of thought to //where// the note shows up. As is, I'm inclined to think that extending the metadata from `std::string` to something richer would be a better design.

Let me know if I'm misunderstanding the code or the intent of the patch.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128807