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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 12 02:57:23 PDT 2018

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?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list