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

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

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Ship it! (but let's drop the extra operator== if possible?)

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:
> > 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.
> > 
> I still find a prefix useful in some cases, e.g. the `unresolved identifier foo. did you mean bar?`.
> Mostly for cases where a fix note uses the error message of the main diagnostic or when it's badly phrased (haven't really seen those, but I would be surprised if there are none).
that's... exactly the case I was replying to :)
I don't think it's common or confusing enough, and there are other fixes when using the main diagnostic. But this isn't a big deal; let's leave it for now.

Comment at: clangd/Protocol.h:166
+inline bool operator==(const TextEdit &LHS, const TextEdit &RHS) {
+  return std::tie(LHS.range, LHS.newText) == std::tie(RHS.range, RHS.newText);
The TextEdit/Diagnostic == operators seem odd - what do we need these for?

Comment at: unittests/clangd/ClangdUnitTests.cpp:70
+class WithFixesMatcher : public testing::MatcherInterface<const Diag &> {
ilya-biryukov wrote:
> sammccall wrote:
> > Ah, and now that you've written this... ;-)
> > 
> > For the specific case of "field X matches matcher M" you can use ::testing::Field (you'd keep WithNote, but make it call Field)
> > 
> > Messages are good as long as you pass the optional field_name param.
> Was a useful exercise anyway.
> Couldn't find the optional 'field_name' param, though, am I missing something?
You're not, sorry - it was added recently to google's copy and hasn't been pushed to upstream gmock yet. So for now we have slightly bad error messages (or have the full MatcherInterface implementation)

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list