[PATCH] D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D50814#1224292, @ilya-biryukov wrote:

> Sorry for the delayed response. It seems we absolutely need this if mirroring  libclang is an absolute requirement.
>  We seem to have multiple implementation options:
>
> Which classes to use for representing diagnostics? We can:
>
> 1. Reuse existing hierarchy for diagnostics. (current option)
> 2. Have separate hierarchies for "flat" (fixits with notes) and "grouped" (fix-its are always attached to main diag). "flat" diagnostics can be stored exactly as they come from clang, with notes and main diags in the same vector, main diag should lack the "notes" field. I.e. if we choose to go with "raw" clang mode, we can as well avoid grouping the notes altogether instead of having code that does something in between the classical libclang and new clangd approach.
>
>   I think the added separation is worth it for code clarity, but also realize it's a bunch of boilerplate, so others may disagree. WDYT?


I think having separate hierarchies would be more confusing and harder to test well. I'd prefer to use one hierarchy.

> Another alternative that we might explore is always producing fix-its attached to notes at the lowest level (in `Diagnostics.cpp`) and reconstructing the current diagnostics hierarchy somewhere higher up the chain, e.g. at the LSPServer level.
>  This would allow to keep the option at the LSPServer level and would only require extracting a (seemingly simple) conversion function that can be called at the LSP Server level.
>  Having the option handled at the top-level seems like a win, since most of the code does not need to care about this option.\

That would be my personal preference. I will work on an updated patch that does that.

> + at sammccall, who wanted to take a look at this issue.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50814