[PATCH] D35894: [clangd] Code hover for Clangd
Marc-Andre Laperle via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 7 14:38:21 PST 2017
malaperle requested changes to this revision.
malaperle added inline comments.
This revision now requires changes to proceed.
================
Comment at: clangd/ClangdLSPServer.cpp:205
+
+ if (!(H.contents[0].codeBlockLanguage == "" &&
+ H.contents[0].markdownString == "" &&
----------------
I think we should just always call unparse and handle the various cases in there, because we always have to return a Hover anyway (see comment below).
================
Comment at: clangd/ClangdLSPServer.cpp:210
+ else
+ C.reply("[]");
+}
----------------
This is an invalid response because we still have to return a Hover, it is not optional. We can return a empty "contents" array though, so I think it would look like: { contents: [] }
================
Comment at: clangd/ClangdServer.cpp:360
+
+ auto FileContents = DraftMgr.getDraft(File);
+ assert(FileContents.Draft &&
----------------
Why are you adding this? Is this coming from another commit?
It looks like it makes tests fail:
<-- {"jsonrpc":"2.0","id":2,"method":"textDocument/definition","params":{"textDocument":{"uri":"file:///doesnotexist.cpp"},"position":{"line":4,"character":7}}}
clangd: ../tools/clang/tools/extra/clangd/ClangdServer.cpp:362: llvm::Expected<clang::clangd::Tagged<std::vector<std::pair<clang::clangd::Location, const clang::Decl*> > > > clang::clangd::ClangdServer::findDefinitions(clang::clangd::PathRef, clang::clangd::Position): Assertion `FileContents.Draft && "findDefinitions is called for non-added document"' failed.
/home/emalape/git/llvm/tools/clang/tools/extra/test/clangd/definitions.test:169:10: error: expected string not found in input
================
Comment at: clangd/ClangdServer.cpp:445
+
+ std::vector<MarkedString> Contents;
+ MarkedString MS = MarkedString("", "");
----------------
I would just remove all this initialization code and leave only Hover FinalHover;
The default constructor of Hover should be equivalent to "empty" result (but still valid). More below.
================
Comment at: clangd/ClangdServer.cpp:446
+ std::vector<MarkedString> Contents;
+ MarkedString MS = MarkedString("", "");
+ Contents.push_back(MS);
----------------
This is the same as the default contructor, so just
MarkedString MS;
would have been OK.
================
Comment at: clangd/ClangdServer.cpp:449
+ Range R;
+ // Hover FinalHover(MS, R);
+ Hover FinalHover(Contents, R);
----------------
remove commented line
================
Comment at: clangd/ClangdServer.cpp:450
+ // Hover FinalHover(MS, R);
+ Hover FinalHover(Contents, R);
+ auto TaggedFS = FSProvider.getTaggedFileSystem(File);
----------------
I actually think you should leave it with the default contructor, so that "contents" is an empty array, which is what should be returned when there is no hover.
================
Comment at: clangd/ClangdServer.cpp:454
+ std::shared_ptr<CppFile> Resources = Units.getFile(File);
+ assert(Resources && "Calling findDefinitions on non-added file");
+
----------------
findDefinitions -> findHover
================
Comment at: clangd/ClangdServer.cpp:456
+
+ std::vector<std::pair<Location, const Decl *>> Result;
+
----------------
You can move this inside the lambda.
================
Comment at: clangd/ClangdServer.cpp:459
+ Resources->getAST().get()->runUnderLock(
+ [Pos, &Result, &FinalHover, this](ParsedAST *AST) {
+ if (!AST)
----------------
Don't need to capture Result if it's declared inside the lambda (previous comment)
================
Comment at: clangd/ClangdServer.cpp:464
+ if (!Result.empty()) {
+ FinalHover = clangd::getHover(*AST, Result[0]);
+ }
----------------
Maybe we should put a comment here that we only show the hover for the first "definition" here and that perhaps more could be shown in the future.
================
Comment at: clangd/ClangdServer.h:67
+
+ // template <class U>
----------------
remove
================
Comment at: clangd/ClangdServer.h:281
/// Get definition of symbol at a specified \p Line and \p Column in \p File.
- llvm::Expected<Tagged<std::vector<Location>>> findDefinitions(PathRef File,
+ llvm::Expected<Tagged<std::vector<std::pair<Location, const Decl*>>>> findDefinitions(PathRef File,
Position Pos);
----------------
Location can be deduced from Decl*. I don't think this should be a pair. I also think we need a more general finder that just gets either the Decl* or MacroDef* at the "cursor". I think CXCursor does something similar in that it stores either Decl* or MacroDef*. I'm not sure it would be worth moving CXCursor from libclang but perhaps something stripped down for Clangd would be OK. This new finder will be able to be reused by findDefinition, highlights, hover, references and more.
We can make one patch that refactors the existing code so that findDefinitions uses this new finder class.
================
Comment at: clangd/ClangdUnit.cpp:941
+
+ // Keep default value.
+ SourceRange SR = D->getSourceRange();
----------------
This code doesn't belong here. The hover feature and "find definition" do not need the same range. The hover basically wants everything up to the first left brace but "find definitions" wants everything up to the closing brace. So having this code here will make "find definition" less correct.
================
Comment at: clangd/ClangdUnit.cpp:942
+ // Keep default value.
+ SourceRange SR = D->getSourceRange();
+
----------------
SR is set but never read. Did you mean to use it below in the DeclarationLocations.push_back?
================
Comment at: clangd/ClangdUnit.cpp:946
+ if (FuncDecl->isThisDeclarationADefinition())
+ {
+ if (FuncDecl->hasBody())
----------------
remove braces
================
Comment at: clangd/ClangdUnit.cpp:947
+ {
+ if (FuncDecl->hasBody())
+ {
----------------
ifs can be combined
================
Comment at: clangd/ClangdUnit.cpp:954
+ } else if (auto ClassDecl = dyn_cast<TagDecl>(D)) {
+ if (ClassDecl->isStruct()) {
+ SR = SourceRange(D->getSourceRange().getBegin(),
----------------
Do you need those checks: isStruct(), isClass and isEnum? Is there a case that ClassDecl->getBraceRange() won't work?
I see those check are tied to the TagTypeKind enum, would __interface and union not be OK for this?
================
Comment at: clangd/ClangdUnit.cpp:955
+ if (ClassDecl->isStruct()) {
+ SR = SourceRange(D->getSourceRange().getBegin(),
+ ClassDecl->getBraceRange().getBegin());
----------------
missing check for isThisDeclarationADefinition ?
For example, getBraceRange will return invalid locations for a forward declaration of a struct:
struct MyStruct;
================
Comment at: clangd/ClangdUnit.cpp:972
+ else
+ SR = D->getSourceRange();
+ } else if (dyn_cast<TypedefDecl>(D)) {
----------------
not needed, already initialized to that.
================
Comment at: clangd/ClangdUnit.cpp:974
+ } else if (dyn_cast<TypedefDecl>(D)) {
+ SR = D->getSourceRange();
+ }
----------------
not needed, already initialized to that.
================
Comment at: clangd/ClangdUnit.cpp:979
+ else if (dyn_cast<ValueDecl>(D)) {
+ SR = D->getSourceRange();
+ }
----------------
not needed, already initialized to that.
================
Comment at: clangd/ClangdUnit.cpp:981
+ }
+
if (isSearchedLocation(FID, Offset)) {
----------------
I think we need to do a check that the computed SourceRange is valid (isValid) in case we get it wrong. Otherwise, it might not go well later when we use the SourceLocation to convert to lines, etc.
================
Comment at: clangd/ClangdUnit.cpp:1116
+ Location L = LocationDecl.first;
+ std::vector<MarkedString> Contents;
+ unsigned Start;
----------------
Not needed, you can use H.contents directly.
================
Comment at: clangd/ClangdUnit.cpp:1117
+ std::vector<MarkedString> Contents;
+ unsigned Start;
+ unsigned End;
----------------
join with definition below.
================
Comment at: clangd/ClangdUnit.cpp:1118
+ unsigned Start;
+ unsigned End;
+
----------------
join with definition below.
================
Comment at: clangd/ClangdUnit.cpp:1121
+ const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+ Range R;
+ const FileEntry *FE = SourceMgr.getFileManager().getFile(L.uri.file);
----------------
remove?
================
Comment at: clangd/ClangdUnit.cpp:1123
+ const FileEntry *FE = SourceMgr.getFileManager().getFile(L.uri.file);
+ FileID id = SourceMgr.translateFile(FE);
+ StringRef Ref = SourceMgr.getBufferData(id);
----------------
FID
================
Comment at: clangd/ClangdUnit.cpp:1179
+ if (!II)
+ Res = "anonymous namespace";
+ else if (Contexts[0]->isStdNamespace())
----------------
this case is already covered above so the string "(anonymous namespace)" is already stored in Res.
================
Comment at: clangd/ClangdUnit.cpp:1181
+ else if (Contexts[0]->isStdNamespace())
+ Res = "std namespace";
+ else
----------------
this case is already covered above so the string "std" is already stored in Res.
================
Comment at: clangd/ClangdUnit.cpp:1183
+ else
+ Res = "namespace named " + Res;
+ } else if (isa<TagDecl>(Contexts[0])) {
----------------
this case is already covered above so the string "ns1::ns2" is already stored in Res.
================
Comment at: clangd/ClangdUnit.cpp:1188
+ {
+ Res = "class named " + Res;
+ } else if (s == "Enum") {
----------------
I don't think we should explicitly say whether or not the symbol is in a namespace, class or enum. Otherwise, it will be complicated and crowded I think. Example, for a nested enum in a class in multiple namespaces: "In namespace named llvm::clang::clangd and class named ClangdServer and enum named MyEnum". I think it's fine to put "In llvm::clang::clangd::ClangdServer::MyEnum". I think later, we could use coloring or style to differentiate between each types.
================
Comment at: clangd/ClangdUnit.cpp:1199
+ if (isa<FunctionDecl>(NamedD)) {
+ MarkedString Info("global function");
+ Contents.push_back(Info);
----------------
Maybe not printing anything could imply global?
================
Comment at: clangd/ClangdUnit.cpp:1212
+
+ MarkedString MS("#define statement");
+ Contents.push_back(MS);
----------------
I'm not sure what to do with those. It's a bit odd to have "define statement" displayed first where the other cases we print the scope. Maybe it would be best to just print from the beginning of the line? So
"#define FOO 1"
instead of
"#define statement
FOO 1"
This would mean a tweak to the translateFileLineCol call above.
Also, the # is screwing up things because it thinks it's markdown. But if you end up removing this "#define statement" then it won't be a problem anyway.
================
Comment at: clangd/ClangdUnit.cpp:1219
+ R = L.range;
+ Hover H(Contents, R);
+ return H;
----------------
You can move up this declaration and use the default constructor, then below you fill the fields as necessary, so you won't need a few other local variables.
================
Comment at: clangd/Protocol.cpp:892
+ llvm::raw_string_ostream(Result) << llvm::format(
+ R"({"contents": [%s], "range": %s})", ContentsStr.c_str(),
+ Range::unparse(H.range).c_str());
----------------
"range" is optional so it should be conditionally outputted here.
================
Comment at: clangd/Protocol.cpp:899
+ std::string Result;
+ if (MS.markdownString != "") {
+ llvm::raw_string_ostream(Result) << llvm::format(
----------------
empty()
================
Comment at: clangd/Protocol.cpp:902
+ R"("%s")", llvm::yaml::escape(MS.markdownString).c_str());
+ } else {
+
----------------
both if and else statements have one like so you can remove the { }
================
Comment at: clangd/Protocol.cpp:906
+ << llvm::format(R"({"language": "%s", "value": "%s"})",
+ (llvm::yaml::escape(MS.codeBlockLanguage)).c_str(),
+ (llvm::yaml::escape(MS.codeBlockValue)).c_str());
----------------
extra parenthesis here
================
Comment at: clangd/Protocol.h:418
+
+ MarkedString(std::string markdown)
+ : markdownString(markdown), codeBlockLanguage(""), codeBlockValue("") {}
----------------
I would remove these constructors, they don't add much and it's inconsistent with the other structs.
================
Comment at: clangd/Protocol.h:426
+ std::string markdownString;
+ std::string codeBlockLanguage;
+ std::string codeBlockValue;
----------------
just "language" ?
================
Comment at: clangd/Protocol.h:427
+ std::string codeBlockLanguage;
+ std::string codeBlockValue;
+
----------------
maybe you can merge markdownString and codeBlockValue into one "value"? Then whether or not "language" is empty will decide if the construct "{language: string; value: string }" is to be used.
================
Comment at: clangd/Protocol.h:434
+
+ Hover(std::vector<MarkedString> contents, Range r)
+ : contents(contents), range(r) {}
----------------
I would remove this constructor, it doesn't add much and it's inconsistent with the other structs.
================
Comment at: clangd/Protocol.h:446
+ */
+ Range range;
+
----------------
llvm::Optional?
https://reviews.llvm.org/D35894
More information about the cfe-commits
mailing list