courbet added a comment. In D128807#3622779 <https://reviews.llvm.org/D128807#3622779>, @ymandel wrote:
> In D128807#3622727 <https://reviews.llvm.org/D128807#3622727>, @li.zhe.hua > wrote: > >> - 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. > > Eric, these are great points. Originally, the idea for note (IIRC) came from > the observation that sometimes a single match will generate edits in multiple > locations, each which deserve a different diagnostic note (perhaps the same > text, but appearing at the respective location). The intent was *not* to > allow noting the insertion of paren, for example. So, I think it was a > mistake. Yes, sometimes an ASTEdit is a coherent change, but sometimes (as > the paren example demonstrates) its just a fragment. > > So, from my original intent, I think that we'd just want to support multiple > notes per rule, with each keyed on a source range. > > Clement -- what was your intended application? That may help clarify. Thank for the comments. 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). Right now I have a check that is implemented as a transformer check. I want to emit an extra note. However, because `transformer` do not support notes I will have to rewrite the check to a traditional non-`transformer` one, which is a large change and could be a one-liner if `transformer` supported notes. In terms of the overall design requirements, non-`transformer` checks can have multiple notes per rule, associated to a source location (and multiple checks are using the note location to point to somewhere else in the code, so that is a required feature if we want to be as powerful as the original clang-tidies, which I think we do). Notes cannot be associated to a particular `FixitHint`, and I'm not sure whether that's useful. I went with the design in this patch (notes associated to edits) because: - it looked like associating a note with an `ASTEdit` was the original intent, given that `ASTEdit` already had a `Note` field. - we can plumb notes inside `Metadata`, but `Metadata` is already used for the warning, so that looks a bit more involved. No strong opinion on that front though :) 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
