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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for picking this up!
I have just one major comment related to how we generate the hover text.

Current implementation tries to get the actual source code for every declaration and then patch it up to look nice on in the output.
It is quite a large piece of code (spanning most of the patch) and tries to mix and match source code and output from the AST when there's not enough information in the AST (e.g. `tryFixupIndentation` and `stripOpeningBracket`). This approach is really complex as we'll inevitably try to parse "parts" of C++ and figure out how to deal with macros, etc.
Worse than that, I would expect this code to grow uncontrollably with time and be very hard to maintain.
Moreover, in presence of macros it arguably gives non-useful results. For example, consider this:

  #define DEFINITIONS int foo(); int bar(); int baz();
  
  // somewhere else
  DEFINITIONS
  
  // somewhere else
  int test() {
    foo(); // <-- hover currently doesn't work here. And even if it did, showing a line with just DEFINITIONS is not that useful.
  }

I suggest we move to a different approach of pretty-printing the relevant parts of the AST. It is already implemented in clang, handles all the cases in the AST (and will be updated along when AST is changed), shows useful information in presence of macros and is much easier to implement.
The version of `getHoverContents` using this is just a few lines of code:

  static std::string getHoverContents(const Decl *D) {
    if (TemplateDecl *TD = D->getDescribedTemplate())
      D = TD; // we want to see the template bits.
  
    std::string Result;
    llvm::raw_string_ostream OS(Result);
  
    PrintingPolicy Policy(D->getASTContext().getLangOpts());
    Policy.TerseOutput = true;
    D->print(OS, Policy);
  
    OS.flush();
    return Result;
  }

It doesn't add the `"Defined in ..."` piece, but illustrates the APIs we can use. We should use the same APIs for the scope as well, avoiding fallbacks to manual printing if we can.
If there's something missing/wrong in the upstream pretty printers, it's fine and we can fix them (e.g., the thing that I've immediately noticed is that namespaces are always printed with curly braces).



================
Comment at: clangd/ClangdLSPServer.cpp:325
+void ClangdLSPServer::onHover(TextDocumentPositionParams &Params) {
+
+  llvm::Expected<Tagged<Hover>> H =
----------------
NIT: remove the empty line at the start of function


================
Comment at: clangd/Protocol.cpp:324
+json::Expr toJSON(const MarkupContent &MC) {
+  const char *KindStr = NULL;
+
----------------
NIT: use nullptr instead of NULL
(irrelevant if we use `StringRef`, see other comment below)


================
Comment at: clangd/Protocol.cpp:331
+
+  switch (MC.kind) {
+  case MarkupKind::PlainText:
----------------
- `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");
}
```


================
Comment at: clangd/XRefs.cpp:326
+  if (const TagDecl *TD = dyn_cast<TagDecl>(ND)) {
+    switch (TD->getTagKind()) {
+    case TTK_Class:
----------------
Same suggestion as before. Could we extract a helper function to mark invalid enum values unreachable?



================
Comment at: clangd/XRefs.cpp:552
+  if (!Decls.empty()) {
+    assert(Decls[0] != nullptr);
+    if (Decls[0] != nullptr)
----------------
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)


================
Comment at: clangd/XRefs.cpp:560
+    assert(Macros[0] != nullptr);
+    if (Macros[0] != nullptr)
+      return getHoverContents(Macros[0], AST);
----------------
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)




================
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.
+#
----------------
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)


================
Comment at: unittests/clangd/XRefsTests.cpp:231
+TEST(Hover, All) {
+
+  struct OneTest {
----------------
NIT: remove empty line at the start of the function


================
Comment at: unittests/clangd/XRefsTests.cpp:233
+  struct OneTest {
+    const char *input;
+    const char *expectedHover;
----------------
NIT: Use `StringRef` instead of `const char*`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894