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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
