ioeric added inline comments.
================ Comment at: clangd/ClangdLSPServer.cpp:194 + if (!Replacements) + return replyError(ErrorCode::InternalError, + llvm::toString(Replacements.takeError())); ---------------- 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. ================ Comment at: clangd/ClangdServer.cpp:209 + auto Action = [Contents, Pos, TaggedFS, + PCHs](Path File, decltype(Callback) Callback, + llvm::Expected<InputsAndPreamble> IP) { ---------------- nit: I'd probably use a different name than `Callback` for this parameter for clarity. ================ 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) { ---------------- Consider spelling out the captured values, just in case new variables which are not expected to be captured are added in the future. ================ 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); ---------------- I'd expect this to be checked by callers. Would `return std::move(Result);` work? 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