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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
