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