[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:
> daiyousei-qz wrote:
> > 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.
> 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?
> 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.

Ok, fair enough. I did some manual testing of the updated patch and reformat() seems to have sane behaviour for expressions.

>From my point of view, this patch is good to go (with the line endings fixed).  However, I will defer approval to @sammccall in case he has additional feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127082