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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082