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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

> 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?

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

Yes, that's the goal, essentially. And the `Note` field came from exactly that observation.

> Notes cannot be associated to a particular `FixitHint`, and I'm not sure whether that's useful.

Right. I'm not sure about that. It seems more intuitive for the note to be bundled with the associated `FixitHint` in the same `DiagnosticBuilder`. That is, if we want to (mostly) go with this design, I would think we'd create a separate diagnostic per note or somesuch.

Overall, I'm fine with adding support for additional notes, but I want to get these details sorted out first, since they subtly change the API.

> 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