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
  // 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);
    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*`

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to