sammccall added a comment.
Sorry, didn't realize this was waiting on me! I like the feature and the
threshold seems like a good idea. People might find it inconsistent, but we'll
have to see.
I like placing it after the macro definition, I think the cases where expansion
is/isn't shown feel more consistent that way.
My main hesitation is that it's nice if the members of HoverInfo are more
generic: it ensures some coherence between different types of hovers and makes
it easier to reason about changes to the Hoverinfo->markdown.
Concretely I wonder whether we can get away with appending this to the
definition as a single block like:
#define DOUBLE(X) X##X
// Expands to:
mm
================
Comment at: clang-tools-extra/clangd/Hover.cpp:679
+ // anyway.
+ std::string ExpansionTextBuffer;
+ ExpansionTextBuffer.reserve(2048);
----------------
nit: "Buffer" is redundant here
================
Comment at: clang-tools-extra/clangd/Hover.cpp:684
+ StringRef ExpandedTokText = ExpandedTok.text(SM);
+ if (ExpansionTextBuffer.size() + ExpandedTokText.size() + 1 <
+ ExpansionTextBuffer.capacity()) {
----------------
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)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1216
+ if (!MacroExpansion.empty()) {
+ Output.addRuler();
+ Output.addParagraph().appendText("Macro Expansion:");
----------------
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)
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1217
+ Output.addRuler();
+ Output.addParagraph().appendText("Macro Expansion:");
+ Output.addCodeBlock(MacroExpansion, DefinitionLanguage);
----------------
I think "Expands to" or "Expanded text" is clearer than "macro expansion" -
expansion names the process rather than the result.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:481
- // macro
+ // variable-like macro
+ {R"cpp(
----------------
nit: these are known as "object-like" macros, I don't really know why
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:490
+ HI.Definition = "#define MACRO 41";
+ HI.MacroExpansion = "41";
+ }},
----------------
this is redundant with the definition, ideally I think we'd omit one (the
output is pretty heavyweight).
It's *almost* correct to omit the expansion for object-like macros, the problem
with this is:
```
#define INDIRECT DOUBLE(y)
#define DOUBLE(X) X##X
int INDIRECT; // we *should* show expansion here
```
Still, this is probably rare enough that it's better to not show expansions for
object-like macros on balance?
(It would be possible to actually check for nested expansions but I doubt it's
worth the complexity, certainly not in this patch)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127082/new/
https://reviews.llvm.org/D127082
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
