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
