penagos added inline comments.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:125
+
CurrentToken->Next->getStartOfNonWhitespace().getLocWithOffset(
+ -1)))
return false;
----------------
krasimir wrote:
> penagos wrote:
> > MyDeveloperDay wrote:
> > > I don't really understand what we are saying here?
> > Effectively we are checking that, barring intervening whitespace, we are
> > analyzing 2 consecutive '>' tokens. If so, we treat such sequence as a
> > binary op in lieu of a closing template angle bracket. If there's another
> > more straightforward way of accomplishing this check, I'm open to that, but
> > this seemed to be the most straightforward way at the time.
> I'm worried that this may regress template code. How does this account for
> cases where two consecutive `>`-s are really two closing template brackets,
> e.g.,
> `std::vector<std::decay_t<int& >> v;`?
>
> In particular, one added test case is ambiguous: `>>` could really be two
> closing template brackets:
> https://godbolt.org/z/v19hj9vKn
>
> I have to say that my general feeling about trying to disambiguate between
> bitshifts and template closers is: don't try too hard inside clang-format as
> the heuristics are generally quite brittle and make the code harder to
> maintain; in cases where clang-format wrongly detects bitshift as templates,
> users should add parens around the bitshift, which IMO improves readability.
As this patch currently stands, it does not disambiguate between bitshift '>>'
operators and 2 closing template brackets, so in your snippet, we would no
longer insert a space between the '>' characters (despite arguably being the
better formatting decision in this case).
I agree with your feeling that user guided disambiguation between bitshift
operators and template closing brackets via parens is the ideal solution and
also improves readability, but IMO the approach taken by clang-format to format
the '>' token should be conservative in that any change made should be
non-semantic altering, which is not presently the case. While the case you
mentioned would regress, we would no longer potentially alter program
semantics. Thinking about this more, would it make sense to modify the actual
white-space change generation later on in the analysis to not break up >>
sequences of characters in lieu of annotating the tokens differently as the
proposed patch is currently doing?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100778/new/
https://reviews.llvm.org/D100778
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
