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

cfe-commits mailing list

Reply via email to