ioeric added a comment. Nice! The code looks much clearer. Just some nits
================ Comment at: clangd/ClangdLSPServer.cpp:156 +void ClangdLSPServer::onExit(ExitParams &Params) { + // XXX handler should return true. +} ---------------- ? ================ Comment at: clangd/ClangdLSPServer.h:45 /// Run LSP server loop, receiving input for it from \p In. \p In must be /// opened in binary mode. Output will be written using Out variable passed to ---------------- doc is outdated now ================ Comment at: clangd/JSONRPCDispatcher.cpp:218 -// Tries to read a line up to and including \n. -// If failing, feof() or ferror() will be set. -static bool readLine(std::FILE *In, std::string &Out) { - static constexpr int BufSize = 1024; - size_t Size = 0; - Out.clear(); - for (;;) { - Out.resize(Size + BufSize); - // Handle EINTR which is sent when a debugger attaches on some platforms. - if (!llvm::sys::RetryAfterSignal(nullptr, ::fgets, &Out[Size], BufSize, In)) - return false; - clearerr(In); - // If the line contained null bytes, anything after it (including \n) will - // be ignored. Fortunately this is not a legal header or JSON. - size_t Read = std::strlen(&Out[Size]); - if (Read > 0 && Out[Size + Read - 1] == '\n') { - Out.resize(Size + Read); - return true; - } - Size += Read; - } -} - -// Returns None when: -// - ferror() or feof() are set. -// - Content-Length is missing or empty (protocol error) -static llvm::Optional<std::string> readStandardMessage(std::FILE *In, - JSONOutput &Out) { - // A Language Server Protocol message starts with a set of HTTP headers, - // delimited by \r\n, and terminated by an empty line (\r\n). - unsigned long long ContentLength = 0; - std::string Line; - while (true) { - if (feof(In) || ferror(In) || !readLine(In, Line)) - return llvm::None; - - Out.mirrorInput(Line); - llvm::StringRef LineRef(Line); - - // We allow comments in headers. Technically this isn't part - // of the LSP specification, but makes writing tests easier. - if (LineRef.startswith("#")) - continue; - - // Content-Length is a mandatory header, and the only one we handle. - if (LineRef.consume_front("Content-Length: ")) { - if (ContentLength != 0) { - elog("Warning: Duplicate Content-Length header received. " - "The previous value for this message ({0}) was ignored.", - ContentLength); - } - llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength); - continue; - } else if (!LineRef.trim().empty()) { - // It's another header, ignore it. - continue; - } else { - // An empty line indicates the end of headers. - // Go ahead and read the JSON. - break; - } - } - - // The fuzzer likes crashing us by sending "Content-Length: 9999999999999999" - if (ContentLength > 1 << 30) { // 1024M - elog("Refusing to read message with long Content-Length: {0}. " - "Expect protocol errors", - ContentLength); - return llvm::None; - } - if (ContentLength == 0) { - log("Warning: Missing Content-Length header, or zero-length message."); - return llvm::None; - } - - std::string JSON(ContentLength, '\0'); - for (size_t Pos = 0, Read; Pos < ContentLength; Pos += Read) { - // Handle EINTR which is sent when a debugger attaches on some platforms. - Read = llvm::sys::RetryAfterSignal(0u, ::fread, &JSON[Pos], 1, - ContentLength - Pos, In); - Out.mirrorInput(StringRef(&JSON[Pos], Read)); - if (Read == 0) { - elog("Input was aborted. Read only {0} bytes of expected {1}.", Pos, - ContentLength); - return llvm::None; - } - clearerr(In); // If we're done, the error was transient. If we're not done, - // either it was transient or we'll see it again on retry. - Pos += Read; - } - return std::move(JSON); -} - -// For lit tests we support a simplified syntax: -// - messages are delimited by '---' on a line by itself -// - lines starting with # are ignored. -// This is a testing path, so favor simplicity over performance here. -// When returning None, feof() or ferror() will be set. -static llvm::Optional<std::string> readDelimitedMessage(std::FILE *In, - JSONOutput &Out) { - std::string JSON; - std::string Line; - while (readLine(In, Line)) { - auto LineRef = llvm::StringRef(Line).trim(); - if (LineRef.startswith("#")) // comment - continue; - - // found a delimiter - if (LineRef.rtrim() == "---") - break; - - JSON += Line; - } - - if (ferror(In)) { - elog("Input error while reading message!"); - return llvm::None; - } else { // Including EOF - Out.mirrorInput( - llvm::formatv("Content-Length: {0}\r\n\r\n{1}", JSON.size(), JSON)); - return std::move(JSON); - } -} - // The use of C-style std::FILE* IO deserves some explanation. // Previously, std::istream was used. When a debugger attached on MacOS, the ---------------- The comment should also be moved? ================ Comment at: clangd/JSONRPCDispatcher.h:29 +// Logs to an output stream, such as stderr. +// TODO: rename and move. class JSONOutput : public Logger { ---------------- nit: make the FIXME more concrete? ================ Comment at: clangd/JSONTransport.cpp:23 + Code = L.Code; + return make_error<StringError>(L.Message, inconvertibleErrorCode()); + })); ---------------- Is `E` guaranteed to be an `LSPError`? Do we need a handler for other errors? ================ Comment at: clangd/JSONTransport.cpp:83 + vlog(Pretty ? "<<< {0:2}\n" : "<<< {0}\n", *Doc); + if (handleMessage(std::move(*Doc), Handler)) + return Error::success(); ---------------- Maybe add a comment here that the return value of `handleMessage` indicates whether to shutdown. It' not clear without context. ================ Comment at: clangd/JSONTransport.cpp:129 + if (!Object || Object->getString("jsonrpc") != Optional<StringRef>("2.0")) { + elog("Not a JSON-RPC message: {0:2}", Message); + return false; ---------------- nit: add `2.0` to the log? ================ Comment at: clangd/Transport.h:50 + virtual ~MessageHandler() = default; + virtual bool notify(llvm::StringRef Method, llvm::json::Value) = 0; + virtual bool call(llvm::StringRef Method, llvm::json::Value Params, ---------------- nit: maybe `handleNotify` to make the two directions more distinguishable? This can get a bit counter-intuitive in the call sites. ================ Comment at: unittests/clangd/JSONTransportTests.cpp:106 + auto Err = T->loop(E); + EXPECT_FALSE(bool(Err)) << llvm::toString(std::move(Err)); + ---------------- Would `operator<<` be used when the error is real (i.e. check passes)? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53286 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits