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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
