[PATCH] D44142: [clangd] Revamp handling of diagnostics.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Not sure if you're waiting on comments from me: the changes look good.
As discussed I still think Fix should be a separate thing with a name because that's how editors treat it, and this is the right layer to decide how to do that mapping.



================
Comment at: clangd/ClangdLSPServer.cpp:329
+          {"title",
+           llvm::formatv("Apply FixIt {0}", GetFixitMessage(D.message))},
           {"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > nit: while here, can we add a colon after FixIt (or if appropriate in practice, just drop the prefix altogether?). My recollection is this is a bit hard to parse.
> I added a colon for now, but happy to look into removing the prefix. The use-case that's slightly confusing is:
> 
> ```use of undeclared identifier 'foo'. did you mean 'bar'?```
> Since the actual fix-it is at the end, it's might be a bit hard to understand.
Yeah. I'd still be inclined to drop the prefix:
 - this case isn't ideal, but is also not terrible
 - this is a "no separate note for fixit" case, where we can consider synthesizing the message from the text edit rather than reusing the diagnostic. That would work well at least in this case.



================
Comment at: clangd/ClangdLSPServer.cpp:329
+          {"title",
+           llvm::formatv("Apply FixIt: {0}", GetFixitMessage(D.message))},
           {"command", ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND},
----------------
nit: fixit --> fix in prefix and in lambda
(And drop the caps in the prefix I think, as this is now a regular english noun rather than a pseudo-product-name)


================
Comment at: clangd/Diagnostics.h:40
+
+struct PersistentDiag {
+  /// Main diagnostic.
----------------
I don't understand what "persistent" means in this context.
This does seem to be an is-a relationship rather than has-a, so I'd find DiagBase/Diag + inheritance clearer.

But with composition, this one seems rike it should be "Diag" and the type of Main could be DiagDetails or so?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44142