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