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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
