Page MenuHomePhabricator
Differential D127082 Diff 444147 clang-tools-extra/clangd/Hover.cpp

Changeset View

Standalone View

View Options

clang-tools-extra/clangd/Hover.cpp

Show First 20 Lines • Show All 635 Lines • ▼ Show 20 Lines if (!ECD->getType()->isDependentType())
HI.Value = toString(ECD->getInitVal(), 10); HI.Value = toString(ECD->getInitVal(), 10);
} }
HI.Definition = printDefinition(D, PP, TB); HI.Definition = printDefinition(D, PP, TB);
return HI; return HI;
} }
/// Generate a \p Hover object given the macro \p MacroDecl. /// Generate a \p Hover object given the macro \p MacroDecl.
HoverInfo getHoverContents(const DefinedMacro &Macro, ParsedAST &AST) { HoverInfo getHoverContents(const syntax::Token &Tok, const DefinedMacro &Macro,
ParsedAST &AST) {
HoverInfo HI; HoverInfo HI;
SourceManager &SM = AST.getSourceManager(); SourceManager &SM = AST.getSourceManager();
HI.Name = std::string(Macro.Name); HI.Name = std::string(Macro.Name);
HI.Kind = index::SymbolKind::Macro; HI.Kind = index::SymbolKind::Macro;
// FIXME: Populate documentation // FIXME: Populate documentation
// FIXME: Populate parameters // FIXME: Populate parameters
// Try to get the full definition, not just the name // Try to get the full definition, not just the name
Show All 12 Lines if (SM.getPresumedLoc(EndLoc, /*UseLineDirectives=*/false).isValid()) {
StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid); StringRef Buffer = SM.getBufferData(SM.getFileID(StartLoc), &Invalid);
if (!Invalid) { if (!Invalid) {
unsigned StartOffset = SM.getFileOffset(StartLoc); unsigned StartOffset = SM.getFileOffset(StartLoc);
unsigned EndOffset = SM.getFileOffset(EndLoc); unsigned EndOffset = SM.getFileOffset(EndLoc);
if (EndOffset <= Buffer.size() && StartOffset < EndOffset) if (EndOffset <= Buffer.size() && StartOffset < EndOffset)
HI.Definition = HI.Definition =
("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset)) ("#define " + Buffer.substr(StartOffset, EndOffset - StartOffset))
.str(); .str();
if (auto Expansion = AST.getTokens().expansionStartingAt(&Tok)) {
// Use a fixed size buffer for better performance and presentation.
nridgeUnsubmitted
Done
ReplyInline Actions

nit: these two lines can be combined as:

if (auto Expansion = AST.getTokens().expansionStartingAt(&Tok)) {
   ...
}
nridge: nit: these two lines can be combined as: ``` if (auto Expansion = AST.getTokens().
// For extremely long expansion text, it's not readable from hover card
// anyway.
std::string ExpansionTextBuffer;
sammccallUnsubmitted
Done
ReplyInline Actions

nit: "Buffer" is redundant here

sammccall: nit: "Buffer" is redundant here
ExpansionTextBuffer.reserve(2048);
for (const auto &ExpandedTok : Expansion->Expanded) {
StringRef ExpandedTokText = ExpandedTok.text(SM);
if (ExpansionTextBuffer.size() + ExpandedTokText.size() + 1 <
sammccallUnsubmitted
Done
ReplyInline Actions

nit: the logic here seems a bit fiddly and micro-optimized - a few reallocations to produce a hover card don't matter.

consider just:

string ExpansionText;
for (tok : Expanded) {
  ExpansionText += ExpandedTok.text(SM);
  if (ExpansionText.size() > 2048) {
    ExpansionText.clear();
    break;
  }
}

(or even extract a function so you can early-return "", which is clearer)

sammccall: nit: the logic here seems a bit fiddly and micro-optimized - a few reallocations to produce a…
ExpansionTextBuffer.capacity()) {
ExpansionTextBuffer += ExpandedTokText;
ExpansionTextBuffer += " ";
} else {
ExpansionTextBuffer.clear();
break;
}
}
HI.MacroExpansion = std::move(ExpansionTextBuffer);
}
} }
} }
return HI; return HI;
} }
std::string typeAsDefinition(const HoverInfo::PrintedType &PType) { std::string typeAsDefinition(const HoverInfo::PrintedType &PType) {
std::string Result; std::string Result;
llvm::raw_string_ostream OS(Result); llvm::raw_string_ostream OS(Result);
▲ Show 20 Lines • Show All 318 Lines • ▼ Show 20 Linesllvm::Optional<HoverInfo> getHover(ParsedAST &AST, Position Pos,
// Macros and deducedtype only works on identifiers and auto/decltype keywords // Macros and deducedtype only works on identifiers and auto/decltype keywords
// respectively. Therefore they are only trggered on whichever works for them, // respectively. Therefore they are only trggered on whichever works for them,
// similar to SelectionTree::create(). // similar to SelectionTree::create().
for (const auto &Tok : TokensTouchingCursor) { for (const auto &Tok : TokensTouchingCursor) {
if (Tok.kind() == tok::identifier) { if (Tok.kind() == tok::identifier) {
// Prefer the identifier token as a fallback highlighting range. // Prefer the identifier token as a fallback highlighting range.
HighlightRange = Tok.range(SM).toCharRange(SM); HighlightRange = Tok.range(SM).toCharRange(SM);
if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) { if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) {
HI = getHoverContents(*M, AST); HI = getHoverContents(Tok, *M, AST);
break; break;
} }
} else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) { } else if (Tok.kind() == tok::kw_auto || Tok.kind() == tok::kw_decltype) {
if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) { if (auto Deduced = getDeducedType(AST.getASTContext(), Tok.location())) {
HI = getDeducedTypeHoverContents(*Deduced, Tok, AST.getASTContext(), PP, HI = getDeducedTypeHoverContents(*Deduced, Tok, AST.getASTContext(), PP,
Index); Index);
HighlightRange = Tok.range(SM).toCharRange(SM); HighlightRange = Tok.range(SM).toCharRange(SM);
break; break;
Show All 34 Lines if (const SelectionTree::Node *N = ST.commonAncestor()) {
// FIXME: support hovers for other nodes? // FIXME: support hovers for other nodes?
// - built-in types // - built-in types
} }
} }
if (!HI) if (!HI)
return llvm::None; return llvm::None;
// Reformat Definition
if (!HI->Definition.empty()) {
auto Replacements = format::reformat( auto Replacements = format::reformat(
Style, HI->Definition, tooling::Range(0, HI->Definition.size())); Style, HI->Definition, tooling::Range(0, HI->Definition.size()));
if (auto Formatted = if (auto Formatted =
tooling::applyAllReplacements(HI->Definition, Replacements)) tooling::applyAllReplacements(HI->Definition, Replacements))
HI->Definition = *Formatted; HI->Definition = *Formatted;
}
// Reformat Macro Expansion
nridgeUnsubmitted
Not Done
ReplyInline Actions

It would be interesting to have a couple of test cases that exercise the reformatting in non-trivial ways, e.g. long expansions that need to be wrapped onto multiple lines

I would suggest two such test cases, one with the expansion being in a declaration context, and the second an expression context (for this one, to make it long enough, the expansion could contain e.g. an a ? b : c expression)

(I'm suggesting the expression-context testcase in part as a sanity check to make sure that format::reformat() handles such code reasonably in the first place)

nridge: It would be interesting to have a couple of test cases that exercise the reformatting in non…
daiyousei-qzAuthorUnsubmitted
Done
ReplyInline Actions

Somehow, this comment goes out of the position.

In my opinion, such test should be written against format::reformat() directly instead of hover message in clangd. One problem is that we are using the current style in users' workspace to reformat the definition/expansion, which means the same tokens might present differently given different .clang-format or fallback style that the user has specified. I do agree that, if tokens don't conform a regular c++ expression, like ) . field, the presentation could be bad. But I suppose there's no obvious solution for that.

daiyousei-qz: Somehow, this comment goes out of the position. In my opinion, such test should be written…
nridgeUnsubmitted
Not Done
ReplyInline Actions

In my opinion, such test should be written against format::reformat() directly instead of hover message in clangd. One problem is that we are using the current style in users' workspace to reformat the definition/expansion, which means the same tokens might present differently given different .clang-format or fallback style that the user has specified. I do agree that, if tokens don't conform a regular c++ expression, like ) . field, the presentation could be bad. But I suppose there's no obvious solution for that.

Ok, fair enough. I did some manual testing of the updated patch and reformat() seems to have sane behaviour for expressions.

From my point of view, this patch is good to go (with the line endings fixed). However, I will defer approval to @sammccall in case he has additional feedback.

nridge: > In my opinion, such test should be written against format::reformat() directly instead of…
nridgeUnsubmitted
Not Done
ReplyInline Actions

Phabricator's inter-diff seems to be broken, but glancing through the new revision, it seems like this comment about adding a couple of more test cases hasn't been addressed yet, are you planning to do that in another update?

nridge: Phabricator's inter-diff seems to be broken, but glancing through the new revision, it seems…
if (!HI->MacroExpansion.empty()) {
auto Replacements =
format::reformat(Style, HI->MacroExpansion,
tooling::Range(0, HI->MacroExpansion.size()));
if (auto Formatted =
tooling::applyAllReplacements(HI->MacroExpansion, Replacements))
HI->MacroExpansion = *Formatted;
}
HI->DefinitionLanguage = getMarkdownLanguage(AST.getASTContext()); HI->DefinitionLanguage = getMarkdownLanguage(AST.getASTContext());
HI->SymRange = halfOpenToRange(SM, HighlightRange); HI->SymRange = halfOpenToRange(SM, HighlightRange);
return HI; return HI;
} }
markup::Document HoverInfo::present() const { markup::Document HoverInfo::present() const {
markup::Document Output; markup::Document Output;
▲ Show 20 Lines • Show All 76 Lines • ▼ Show 20 Lines if (CalleeArgInfo) {
if (CallPassType->Converted && CalleeArgInfo->Type) if (CallPassType->Converted && CalleeArgInfo->Type)
OS << " (converted to " << CalleeArgInfo->Type->Type << ")"; OS << " (converted to " << CalleeArgInfo->Type->Type << ")";
Output.addParagraph().appendText(OS.str()); Output.addParagraph().appendText(OS.str());
} }
if (!Documentation.empty()) if (!Documentation.empty())
parseDocumentation(Documentation, Output); parseDocumentation(Documentation, Output);
if (!Definition.empty()) { if (!Definition.empty()) {
sammccallUnsubmitted
Not Done
ReplyInline Actions

Sorry, I think I wasn't clear about what I was asking for...
Rather than merging HoverInfo::MacroExpansion and HoverInfo::Definition during present(), I'd really like to eliminate HoverInfo::MacroExpansion entirely, and have HoverInfo::Definition be:

#define FOO BAR

// Expands to
BAR

This isn't completely consistent with the way scope/access are handled, but those are somewhat more generic features and I'd rather avoid adding new HoverInfo members specific to macros.

(The final presentation does look good, so this is just about the intermediate HoverInfo struct)

sammccall: Sorry, I think I wasn't clear about what I was asking for... Rather than merging `HoverInfo…
daiyousei-qzAuthorUnsubmitted
Done
ReplyInline Actions

I see. Though I'm not sure if putting both of them in a single string will cause problems when doing the format. That is:

#define MACRO token token token token token

// Expands to
token token token token token token token token token

as a whole will be reformatted by clang-format and I'm not sure if the overall layout could always be preserved. Let me do some expriment tomorrow.

daiyousei-qz: I see. Though I'm not sure if putting both of them in a single string will cause problems when…
Output.addRuler(); Output.addRuler();
std::string ScopeComment; std::string ScopeComment;
// Drop trailing "::". // Drop trailing "::".
if (!LocalScope.empty()) { if (!LocalScope.empty()) {
// Container name, e.g. class, method, function. // Container name, e.g. class, method, function.
// We might want to propagate some info about container type to print // We might want to propagate some info about container type to print
// function foo, class X, method X::bar, etc. // function foo, class X, method X::bar, etc.
ScopeComment = ScopeComment =
"// In " + llvm::StringRef(LocalScope).rtrim(':').str() + '\n'; "// In " + llvm::StringRef(LocalScope).rtrim(':').str() + '\n';
} else if (NamespaceScope && !NamespaceScope->empty()) { } else if (NamespaceScope && !NamespaceScope->empty()) {
ScopeComment = "// In namespace " + ScopeComment = "// In namespace " +
llvm::StringRef(*NamespaceScope).rtrim(':').str() + '\n'; llvm::StringRef(*NamespaceScope).rtrim(':').str() + '\n';
} }
std::string DefinitionWithAccess = !AccessSpecifier.empty() std::string DefinitionWithAccess = !AccessSpecifier.empty()
? AccessSpecifier + ": " + Definition ? AccessSpecifier + ": " + Definition
: Definition; : Definition;
// Note that we don't print anything for global namespace, to not annoy // Note that we don't print anything for global namespace, to not annoy
// non-c++ projects or projects that are not making use of namespaces. // non-c++ projects or projects that are not making use of namespaces.
Output.addCodeBlock(ScopeComment + DefinitionWithAccess, Output.addCodeBlock(ScopeComment + DefinitionWithAccess,
DefinitionLanguage); DefinitionLanguage);
} }
if (!MacroExpansion.empty()) {
Output.addRuler();
sammccallUnsubmitted
Done
ReplyInline Actions

I think the ruler + paragraph header might appear too heavyweight, would need to take a look.

I think we're missing tests for present().
(If this can become part of the definition, then no need for new tests)

sammccall: I think the ruler + paragraph header might appear too heavyweight, would need to take a look.
Output.addParagraph().appendText("Macro Expansion:");
sammccallUnsubmitted
Done
ReplyInline Actions

I think "Expands to" or "Expanded text" is clearer than "macro expansion" - expansion names the process rather than the result.

sammccall: I think "Expands to" or "Expanded text" is clearer than "macro expansion" - expansion names the…
Output.addCodeBlock(MacroExpansion, DefinitionLanguage);
}
return Output; return Output;
} }
// If the backtick at `Offset` starts a probable quoted range, return the range // If the backtick at `Offset` starts a probable quoted range, return the range
// (including the quotes). // (including the quotes).
llvm::Optional<llvm::StringRef> getBacktickQuoteRange(llvm::StringRef Line, llvm::Optional<llvm::StringRef> getBacktickQuoteRange(llvm::StringRef Line,
unsigned Offset) { unsigned Offset) {
assert(Line[Offset] == '`'); assert(Line[Offset] == '`');
▲ Show 20 Lines • Show All 93 Lines • Show Last 20 Lines