[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:245
+
+  C.reply(Hover::unparse(H->Value));
+}
----------------
we need to check the "Expected" here, so

  if (!H) {
    C.replyError(ErrorCode::InternalError,
                 llvm::toString(H.takeError()));
    return;
  }

Unless you can think of other error situations?


================
Comment at: clangd/ClangdServer.cpp:448
   });
+
   return make_tagged(std::move(Result), TaggedFS.Tag);
----------------
revert


================
Comment at: clangd/ClangdServer.cpp:450
 }
-
 llvm::Optional<Path> ClangdServer::switchSourceHeader(PathRef Path) {
----------------
revert


================
Comment at: clangd/ClangdUnit.cpp:943
 
-/// Finds declarations locations that a given source location refers to.
-class DeclarationLocationsFinder : public index::IndexDataConsumer {
-  std::vector<Location> DeclarationLocations;
+/// Finds declarations  and macros that a given source location refers to.
+class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
----------------
extra space before the "and"


================
Comment at: clangd/ClangdUnit.cpp:1092
+  // Default Hover values
+  std::vector<MarkedString> EmptyVector;
+  Hover H;
----------------
not used, remove


================
Comment at: clangd/ClangdUnit.cpp:1094
+  Hover H;
+  StringRef Ref;
+
----------------
Can this be named something more descriptive? HoverText?


================
Comment at: clangd/ClangdUnit.cpp:1117
+
+  if (!DeclVector.empty()) {
+    const Decl *LocationDecl = DeclVector[0];
----------------
I think we need to simplify this part a bit. I'll try to find a way. Feel free to wait until more comments before updating.


================
Comment at: clangd/ClangdUnit.cpp:1173
+        H.range = L.range;
+        Ref = getDataBufferFromSourceRange(AST, SR, L);
+      }
----------------
I get the same crash as I mentioned before if I hover on the class in "isa<ObjCMethodDecl>(D)", from Clangdunit.cpp (disclaimer: I'm testing with older source so it might not be there anymore.)

I have still yet to investigate this.


================
Comment at: clangd/ClangdUnit.cpp:1238
+/// Returns the file buffer data that the given SourceRange points to.
+StringRef clangd::getDataBufferFromSourceRange(ParsedAST &AST, SourceRange SR,
+                                               Location L) {
----------------
getBufferDataFromSourceRange, to be consistent with getSourceManager().getBufferData


================
Comment at: clangd/ClangdUnit.cpp:1239
+StringRef clangd::getDataBufferFromSourceRange(ParsedAST &AST, SourceRange SR,
+                                               Location L) {
+  StringRef Ref;
----------------
I think the SourceRange SR should be used here, not Location.


================
Comment at: clangd/ClangdUnit.h:318
+
+std::string getScope(const NamedDecl *ND, const LangOptions &LangOpts);
+
----------------
I think this can be only in the source file, in a an anonymous namespace.


================
Comment at: clangd/Protocol.cpp:1029
+        {"contents", json::ary(H.contents)},
+        {"range", DefaultRange},
+    };
----------------
I don't think "range" should be outputted here


================
Comment at: test/clangd/hover.test:10
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"#define MACRO 1\nnamespace ns1 {\nint test = 5;\nstruct MyClass {\nint xasd;\nvoid anotherOperation() {\n}\nstatic int foo(MyClass*) {\nreturn 0;\n}\n\n};}\nint main() {\nint a;\na;\nint b = ns1::test;\nns1::MyClass* Params;\nParams->anotherOperation();\nMACRO;}\n"}}}
+
----------------
probably the test should include templates and comments?


================
Comment at: test/clangd/hover.test:15
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":12}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0}}}}
----------------
copy/pasted comment?


================
Comment at: test/clangd/hover.test:21
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":14,"character":1}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"int a"}],"range":{"end":{"character":5,"line":13},"start":{"character":0,"line":13}}}}
----------------
copy/pasted comment?


================
Comment at: test/clangd/hover.test:27
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":15,"character":15}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"int test = 5"}],"range":{"end":{"character":12,"line":2},"start":{"character":0,"line":2}}}}
----------------
copy/pasted comment?


================
Comment at: test/clangd/hover.test:33
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":16,"character":10}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1"},{"language":"C++","value":"struct MyClass {"}],"range":{"end":{"character":16,"line":3},"start":{"character":0,"line":3}}}}
----------------
copy/pasted comment?


================
Comment at: test/clangd/hover.test:39
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":17,"character":13}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"In ns1::MyClass"},{"language":"C++","value":"void anotherOperation() {"}],"range":{"end":{"character":25,"line":5},"start":{"character":0,"line":5}}}}
----------------
copy/pasted comment?


================
Comment at: test/clangd/hover.test:45
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":18,"character":1}}}
+# Go to local variable
+# CHECK: {"id":1,"jsonrpc":"2.0","result":{"contents":[{"language":"C++","value":"#define MACRO 1"}],"range":{"end":{"character":15,"line":0},"start":{"character":8,"line":0}}}}
----------------
copy/pasted comment?


https://reviews.llvm.org/D35894