[PATCH] D35894: [clangd] Code hover for Clangd
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 18 03:56:54 PST 2017
ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:284
+void ClangdLSPServer::onCodeHover(Ctx C, TextDocumentPositionParams &Params) {
+
+ auto H = Server.findHover(C,
----------------
NIT: remove empty line
================
Comment at: clangd/ClangdServer.cpp:428
std::vector<Location> Result;
- Resources->getAST().get()->runUnderLock([Pos, &Result, &Ctx](ParsedAST *AST) {
+ Resources->getAST().get()->runUnderLock([Pos, &Result, &Ctx, this](ParsedAST *AST) {
if (!AST)
----------------
We don't need to capture `this`, remove it from the capture list.
================
Comment at: clangd/ClangdServer.cpp:550
std::shared_ptr<CppFile> Resources = Units.getFile(File);
- if (!Resources)
- return llvm::make_error<llvm::StringError>(
- "findDocumentHighlights called on non-added file",
- llvm::errc::invalid_argument);
-
- std::vector<DocumentHighlight> Result;
- llvm::Optional<llvm::Error> Err;
- Resources->getAST().get()->runUnderLock([Pos, &Ctx, &Err,
- &Result](ParsedAST *AST) {
- if (!AST) {
- Err = llvm::make_error<llvm::StringError>("Invalid AST",
- llvm::errc::invalid_argument);
- return;
- }
- Result = clangd::findDocumentHighlights(Ctx, *AST, Pos);
+ assert(Resources && "Calling findHover on non-added file");
+ Resources->getAST().get()->runUnderLock(
----------------
Return an error instead of asserting to follow how other functions do that:
```
if (!Resources)
return llvm::make_error<llvm::StringError>(
"findHover called on non-added file",
llvm::errc::invalid_argument);
```
================
Comment at: clangd/ClangdServer.h:303
+ llvm::Expected<Tagged<Hover>> findHover(const Context &Ctx, PathRef File, Position Pos);
+
----------------
Please put this right after `findDocumentHighlights`. It is really weird to see `findHover` in the middle of formatting functions.
================
Comment at: clangd/ClangdServer.h:306
+ /// Run formatting for \p Rng inside \p File.
+ std::vector<tooling::Replacement> formatRange(PathRef File, Range Rng);
+ /// Run formatting for the whole \p File.
----------------
These functions are duplicates of existing ones, they should not be here.
Bad merge?
================
Comment at: clangd/ClangdUnit.cpp:222
return Future.wait_for(std::chrono::seconds(0)) == std::future_status::ready;
+
}
----------------
NIT: remove the empty line
================
Comment at: clangd/ClangdUnit.cpp:288
+/// Finds declarations and macros that a given source location refers to.
+
class DeclarationAndMacrosFinder : public index::IndexDataConsumer {
----------------
NIT: remove the empty line
================
Comment at: clangd/ClangdUnit.cpp:303
std::vector<const Decl *> takeDecls() {
- // Don't keep the same declaration multiple times.
+ // Don't keep the same location multiple times.
// This can happen when nodes in the AST are visited twice.
----------------
Bad merge? This comment should not be changed.
================
Comment at: clangd/ClangdUnit.cpp:308
Decls.erase(Last, Decls.end());
- return std::move(Decls);
+ return Decls;
}
----------------
`std::move(Decls)` is better. Bad merge?
================
Comment at: clangd/ClangdUnit.cpp:313
// Don't keep the same Macro info multiple times.
+ // This can happen when nodes in the AST are visited twice.
std::sort(MacroInfos.begin(), MacroInfos.end());
----------------
This comment is not upstream now and we're removed it in the other patch. Remove it here too.
================
Comment at: clangd/ClangdUnit.cpp:438
+ // trimming the extra "::"
+ std::string Res;
+ PrintingPolicy WithScopePP(LangOpts);
----------------
We should avoid doing the string-matching when we have the AST.
Use something like this instead (haven't tested, but you should get the idea):
```
/// Returns a NamedDecl that could be used to print the qualifier.
Decl* getScopeOfDecl(Decl* D) {
// Code from NamedDecl::printQualifiedName, we should probably move it into
const DeclContext *Ctx = D->getDeclContext();
// For ObjC methods, look through categories and use the interface as context.
if (auto *MD = dyn_cast<ObjCMethodDecl>(D))
if (auto *ID = MD->getClassInterface())
Ctx = ID;
if (Ctx->isFunctionOrMethod()) {
/// This is a function-local entity.
return nullptr;
}
return Ctx;
}
std::string printScopeOfDecl(Decl* Scope) {
if (isa<TranslationUnitDecl>(Scope))
return "global namespace";
if (isa<NamedDecl>(Scope))
return cast<NamedDecl>(Scope)->printQualifiedName();
return "";
}
// Later use like this:
auto ScopeText = printScopeOfDecl(getScopeOfDecl(D));
```
================
Comment at: clangd/ClangdUnit.cpp:462
+/// Returns the file buffer data that the given SourceRange points to.
+llvm::ErrorOr<StringRef> getBufferDataFromSourceRange(ParsedAST &AST,
+ Location L) {
----------------
NIT: `llvm::Expected` instead of `llvm::ErrorOr`
================
Comment at: clangd/ClangdUnit.cpp:485
-llvm::Optional<Location>
+llvm::ErrorOr<Location>
getDeclarationLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
----------------
NIT: llvm::Expected instead of llvm::ErrorOr
================
Comment at: clangd/ClangdUnit.cpp:498
Position Begin;
- Begin.line = SourceMgr.getSpellingLineNumber(LocStart) - 1;
- Begin.character = SourceMgr.getSpellingColumnNumber(LocStart) - 1;
+ Begin.line = SourceMgr.getExpansionLineNumber(LocStart) - 1;
+ Begin.character = SourceMgr.getExpansionColumnNumber(LocStart) - 1;
----------------
Why are we changing the semantics here? Why should we use expansion locations instead of spelling locations here?
================
Comment at: clangd/ClangdUnit.cpp:591
+// one line at a time to determine what will be included.
+SourceLocation getFunctionComments(ParsedAST &AST, const Decl *D) {
+
----------------
This code seems pretty involved. Don't `RawComment`s handle this case?
================
Comment at: clangd/ClangdUnit.cpp:658
+ const Decl *LocationDecl = Decls[0];
+ std::vector<MarkedString> HoverContents;
+
----------------
Body of this if statement is a great candidate to be extracted into a separate function. Same for macros. The code is really hard to read as it is right now.
```
std::vector<MarkedString> getHoverContents(Decl *D);
std::vector<MarkedString> getHoverContents(MacroDef *M);
````
================
Comment at: clangd/ClangdUnit.cpp:670
+ // Adjust beginning of hover text depending on the presence of templates and comments.
+ TemplateDecl * TDec = LocationDecl->getDescribedTemplate();
+ const Decl * BeginDecl = TDec ? TDec : LocationDecl;
----------------
the indent is off here. Use clang-format.
================
Comment at: clangd/ClangdUnit.cpp:968
// createInvocationFromCommandLine sets DisableFree.
+ CI->LangOpts->CommentOpts.ParseAllComments = true;
CI->getFrontendOpts().DisableFree = false;
----------------
This should clearly go before the comment!
Also, we're currently setting this flag when building the preamble, but not when parsing the AST. We should definitely do that in both cases.
================
Comment at: clangd/Protocol.h:401
+struct MarkedString {
+
+ /// MarkedString can be used to render human readable text. It is either a
----------------
NIT: remove empty line
================
Comment at: clangd/Protocol.h:427
+struct Hover {
+
+ ///
----------------
NIT: remove empty line
================
Comment at: clangd/Protocol.h:430
+ /// The hover's content
+ ///
+ std::vector<MarkedString> contents;
----------------
NIT: remove empty comment line
================
Comment at: clangd/Protocol.h:436
+ /// that is used to visualize a hover, e.g. by changing the background color.
+ ///
+ llvm::Optional<Range> range;
----------------
NIT: remove empty comment line
================
Comment at: test/clangd/initialize-params-invalid.test:34
# CHECK-NEXT: },
+# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "renameProvider": true,
----------------
indent is off by one
================
Comment at: test/clangd/initialize-params.test:34
# CHECK-NEXT: },
+# CHECK-NEXT: "hoverProvider": true,
# CHECK-NEXT: "renameProvider": true,
----------------
indent is off by one
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D35894
More information about the cfe-commits
mailing list