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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the update!

In D127082#3606746 <https://reviews.llvm.org/D127082#3606746>, @daiyousei-qz wrote:

> Therefore, I still put definition in front of the expansion in this update.
>
> Regarding to the size limit of expansion:
> Here I'm using a 2048 bytes buffer.

I think these are reasonable choices to start with. We can always tweak them after the fact if we'd like.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:676
+      auto Expansion = AST.getTokens().expansionStartingAt(&Tok);
+      if (Expansion) {
+        // Use a fixed size buffer for better performance and presentation.
----------------
nit: these two lines can be combined as:

```
if (auto Expansion = AST.getTokens().expansionStartingAt(&Tok)) {
   ...
}
```


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1091
+
+  // Reformat Macro Expansion
+  if (!HI->MacroExpansion.empty()) {
----------------
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127082/new/

https://reviews.llvm.org/D127082