simark added inline comments.
================ Comment at: clangd/ClangdLSPServer.cpp:325 +void ClangdLSPServer::onHover(TextDocumentPositionParams &Params) { + + llvm::Expected<Tagged<Hover>> H = ---------------- ilya-biryukov wrote: > NIT: remove the empty line at the start of function Done. ================ Comment at: clangd/Protocol.cpp:324 +json::Expr toJSON(const MarkupContent &MC) { + const char *KindStr = NULL; + ---------------- ilya-biryukov wrote: > NIT: use nullptr instead of NULL > (irrelevant if we use `StringRef`, see other comment below) Ack. ================ Comment at: clangd/Protocol.cpp:331 + + switch (MC.kind) { + case MarkupKind::PlainText: ---------------- ilya-biryukov wrote: > - `StringRef` is a nicer abstraction, maybe use it instead of `const char*`? > - Maybe move declaration of `KindStr` closer to its usage? > - Maybe extract a helper function to mark the code path for invalid kinds as > unreachable? I.e. > ``` > static StringRef toTextKind(MarkupKind Kind) { > switch (Kind) { > case MarkupKind::PlainText: > return "plaintext"; > case MarkupKind::Markdown: > return "markdown"; > } > llvm_unreachable("Invalid MarkupKind"); > } > ``` Done. ================ Comment at: clangd/XRefs.cpp:326 + if (const TagDecl *TD = dyn_cast<TagDecl>(ND)) { + switch (TD->getTagKind()) { + case TTK_Class: ---------------- ilya-biryukov wrote: > Same suggestion as before. Could we extract a helper function to mark invalid > enum values unreachable? > Done. ================ Comment at: clangd/XRefs.cpp:552 + if (!Decls.empty()) { + assert(Decls[0] != nullptr); + if (Decls[0] != nullptr) ---------------- ilya-biryukov wrote: > This assert seems rather unlikely and just makes the code more complex. Let's > assume it's true and remove it (along with the subsequent if statement) Done... ================ Comment at: clangd/XRefs.cpp:560 + assert(Macros[0] != nullptr); + if (Macros[0] != nullptr) + return getHoverContents(Macros[0], AST); ---------------- ilya-biryukov wrote: > This assert seems rather unlikely and just makes the code more complex. Let's > assume it's true and remove it (along with the subsequent if statement) > > ... and done. ================ Comment at: test/clangd/hover.test:2 +# RUN: clangd -run-synchronously < %s | FileCheck %s +# It is absolutely vital that this file has CRLF line endings. +# ---------------- ilya-biryukov wrote: > We have a more readable format for the lit-tests now. Could we update this > test to use it? > (see other tests in test/clangd for example) Indeed the new format looks much more readable. I'll modify this one to look like completion.test. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D35894 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits