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

[Qingyuan Zheng via Phabricator via cfe-commits]

daiyousei-qz added a comment.

It looks like my inline comment wasn't submitted (didn't click the submit button in the bottom). Here's my old comment.



================
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)
Somehow, this comment goes out of the position. 

In my opinion, such test should be written against `format::reformat()` directly instead of hover message in clangd. One problem is that we are using the current style in users' workspace to reformat the definition/expansion, which means the same tokens might present differently given different `.clang-format` or fallback style that the user has specified. I do agree that, if tokens don't conform a regular c++ expression, like `) . field`, the presentation could be bad. But I suppose there's no obvious solution for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082