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

[Clement Courbet via Phabricator via cfe-commits]

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