ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:194
+ if (!Replacements)
+ return replyError(ErrorCode::InternalError,
+ llvm::toString(Replacements.takeError()));
----------------
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.
================
Comment at: clangd/ClangdServer.cpp:209
+ auto Action = [Contents, Pos, TaggedFS,
+ PCHs](Path File, decltype(Callback) Callback,
+ llvm::Expected<InputsAndPreamble> IP) {
----------------
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?
================
Comment at: clangd/ClangdServer.cpp:441
- using RetType = llvm::Expected<Tagged<std::vector<DocumentHighlight>>>;
- auto Action = [=](llvm::Expected<InputsAndAST> InpAST) -> RetType {
+ auto Action = [=](decltype(Callback) Callback,
+ llvm::Expected<InputsAndAST> InpAST) {
----------------
ioeric wrote:
> Consider spelling out the captured values, just in case new variables which
> are not expected to be captured are added in the future.
Thanks for spotting this!
================
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);
----------------
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`
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
