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

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.


================
Comment at: clangd/ClangdLSPServer.cpp:228
                         llvm::toString(Items.takeError()));
+
   C.reply(json::ary(Items->Value));
----------------
remove extra line


================
Comment at: clangd/ClangdUnit.cpp:1054
+    return llvm::errc::no_such_file_or_directory;
+  } else {
+    FileID FID = AST.getASTContext().getSourceManager().translateFile(F);
----------------
else is not needed because it returns in the if


================
Comment at: clangd/ClangdUnit.cpp:1080
+    return llvm::errc::no_such_file_or_directory;
+  } else {
+    SourceLocation LocEnd = Lexer::getLocForEndOfToken(ValSourceRange.getEnd(),
----------------
else is not needed since you return in the if


================
Comment at: clangd/ClangdUnit.cpp:1209
+            H.range = L->range;
+          else
+            H.range = Range();
----------------
```
 else
            H.range = Range();
```
Remove this, once the bug in Protocol.cpp is fixed, the Range will be truly optional.


================
Comment at: clangd/ClangdUnit.cpp:1247
+            H.range = L->range;
+          else
+            H.range = Range();
----------------
```
 else
            H.range = Range();
```
Remove this, once the bug in Protocol.cpp is fixed, the Range will be truly optional.


================
Comment at: clangd/ClangdUnit.cpp:1505
           *CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine, VFS, PCHs,
-          /*StoreInMemory=*/That->StorePreamblesInMemory,
-          SerializedDeclsCollector);
+          /*StoreInMemory=*/true, SerializedDeclsCollector);
 
----------------
revert this change


================
Comment at: clangd/Protocol.cpp:498
+    // Default Hover values
+    Hover H;
+    return json::obj{
----------------
remove, we have to return the contents of the H that was passed as parameter, not a new one. I hit this bug while testing with no range (hover text in another file)

So this should be
```
if (H.range.hasValue()) {
    return json::obj{
        {"contents", json::ary(H.contents)},
        {"range", H.range.getValue()},
    };
  }

  return json::obj{
    {"contents", json::ary(H.contents)},
};
```


================
Comment at: clangd/Protocol.h:26
 #include "llvm/ADT/Optional.h"
-#include <string>
+#include "llvm/Support/YAMLParser.h"
 #include <vector>
----------------
revert this change?


================
Comment at: clangd/Protocol.h:463
+
+  /**
+   * The hover's content
----------------
Documentation should use /// like the others


================
Comment at: clangd/Protocol.h:468
+
+  /**
+   * An optional range is a range inside a text document
----------------
Documentation should use /// like the others


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D35894