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