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

[Sam McCall via Phabricator via cfe-commits]

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 &> {
+public:
----------------
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)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44142