ioeric added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:194
+ if (!Replacements)
+ return replyError(ErrorCode::InternalError,
+ llvm::toString(Replacements.takeError()));
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > nit: since we are not spelling out the return type here, it might be
> > clearer if we do:
> > ```
> > replyError(...);
> > return;
> > ```
> >
> > `return replyError(...)` makes me wonder what the return type is.
> This trick was used in the code before (there are usages below), so I figured
> it's ok to do this.
> If it's confusing I'll change it everywhere.
I guess it's not confusing after figuring out what `replyError` returns. There
is obviously tradeoff between LOC and readability. Either is not too bad. So up
to you :)
================
Comment at: clangd/ClangdServer.cpp:209
+ auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+ llvm::Expected<InputsAndPreamble> IP) {
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > nit: I'd probably use a different name than `Callback` for this parameter
> > for clarity.
> I actually think we should keep it this way. It's a bit tricky to grasp, but
> it has two important advantages:
> - Makes referencing `Callback` from outer scope impossible
> - saves us from coming up with names for that superficial variable, i.e.
> should it be called `InnerCallback`, `Callback2`, `C` or something else?
>
> Is it that too confusing or do you feel we can keep it?
> Makes referencing Callback from outer scope impossible.
For lambdas, I think we should rely on captures for this instead of the
parameter names.
>saves us from coming up with names for that superficial variable, i.e. should
>it be called InnerCallback, Callback2, C or something else?
I thought this is a common practice with llvm coding style :P I would vote for
`C` :)
> Is it that too confusing or do you feel we can keep it?
It is a bit confusing when I first looked at it, but nothing is *too* confusing
as long as the code is correct ;) I just think it would make the code easier to
read.
================
Comment at: unittests/clangd/SyncAPI.cpp:69
+ llvm::Expected<Tagged<SignatureHelp>> Result = Tagged<SignatureHelp>();
+ (void)(bool)Result; // Expected has to be checked.
+ Server.signatureHelp(File, Pos, capture(Result), OverridenContents);
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > I'd expect this to be checked by callers. Would `return std::move(Result);`
> > work?
> The reason why we need it is because `capture(Result)` writes return value of
> the callback to the `Result` variable. But we have to first check the
> default-constructed value that was there **before** calling
> `Server.signatureHelp`
I think we should probably fix the behavior of `capture` since this changes the
behavior of `llvm::Expected` in user code - the check is there to make sure
users always check the status. But here we always do this for users.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43227
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
