[PATCH] D127082: [clangd] Add Macro Expansion to Hover

[Sam McCall via Phabricator via cfe-commits]

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