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

Nathan Ridge via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 28 01:22:35 PDT 2022

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)

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list