[PATCH] D35894: [clangd] Code hover for Clangd

[Simon Marchi via Phabricator via cfe-commits]

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