ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
I haven't looked into the `getHover` method's body yet, but here are some
comments about other parts.
Good work, BTW, this looks close to being ready.
================
Comment at: clangd/ClangdServer.cpp:511
+Tagged<Hover> ClangdServer::findHover(PathRef File, Position Pos) {
+
+ //Default Hover values
----------------
NIT: could you remove the empty newlines at the start of the methods?
================
Comment at: clangd/ClangdServer.cpp:514
+ std::vector<MarkedString> EmptyVector;
+ Position BeginPosition = {0,0};
+ Position EndPosition = {0,0};
----------------
Let's follow error-reporting pattern used in other `ClangdServer` features.
We should return `llvm::Expected` and do `Ctx.replyError` on error return
values.
================
Comment at: clangd/ClangdUnit.cpp:959
+ std::sort(DeclarationDecls.begin(), DeclarationDecls.end());
+ auto last = std::unique(DeclarationDecls.begin(), DeclarationDecls.end());
+ DeclarationDecls.erase(last, DeclarationDecls.end());
----------------
NIT: local vars are `UpperCamelCase`. (Not introduced by this commit, but
probably a good place to fix the naming anyway).
================
Comment at: clangd/ClangdUnit.cpp:967
+ // This can happen when nodes in the AST are visited twice.
+ std::sort(DeclarationMacroInfs.begin(), DeclarationMacroInfs.end());
auto last =
----------------
NIT: local vars are `UpperCamelCase`. (Not introduced by this commit, but
probably a good place to fix the naming anyway).
================
Comment at: clangd/ClangdUnit.cpp:1100
+
+ const SourceManager &SM = AST.getASTContext().getSourceManager();
+ const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
----------------
I think this functionality is already implemented in clang. Could you use
`ASTContext::getRawCommentForAnyRedecl`?
================
Comment at: clangd/ClangdUnit.cpp:1199
+ } else if (dyn_cast<TypedefDecl>(LocationDecl)) {
+ // TODO: find a way to get the hover for the type that is being aliased
+ SR = LocationDecl->getSourceRange();
----------------
NIT: we use `FIXME`
================
Comment at: clangd/ClangdUnit.h:323
+
+Location getDeclarationLocation(ParsedAST &AST, const SourceRange
&ValSourceRange);
+
----------------
Could we put `getDeclarationLocation` and `getFunctionComments` in the cpp file
instead? They're just implementation detail of `findDefinitions`/`getHover`,
right?
================
Comment at: clangd/MyClass.cpp:1
+#include "MyClass.h"
+
----------------
Remove it.
================
Comment at: clangd/Protocol.h:26
#include "llvm/ADT/Optional.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Optional.h"
----------------
We don't need `SourceLocation` here
================
Comment at: clangd/Protocol.h:27
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Optional.h"
#include "llvm/Support/YAMLParser.h"
----------------
This include is redundant
https://reviews.llvm.org/D35894
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
