[clang] [clang-format] Fix a bug in annotating angles containing FatArrow (PR #108671)

[kadir çetinkaya via cfe-commits]

================
@@ -243,14 +244,16 @@ class AnnotatingParser {
       // operator that was misinterpreted because we are parsing template
       // parameters.
       // FIXME: This is getting out of hand, write a decent parser.
-      if (InExpr && !Line.startsWith(tok::kw_template) &&
+      if (InExpr && !SeenFatArrow && !Line.startsWith(tok::kw_template) &&
           Prev.is(TT_BinaryOperator)) {
         const auto Precedence = Prev.getPrecedence();
         if (Precedence > prec::Conditional && Precedence < prec::Relational)
           return false;
       }
       if (Prev.isOneOf(tok::question, tok::colon) && !Style.isProto())
         SeenTernaryOperator = true;
+      else if (Prev.is(TT_FatArrow))
----------------
kadircet wrote:

> This patch IMO is a narrowed down version of the other patch https://github.com/llvm/llvm-project/pull/100980, which fixed a real regression https://github.com/llvm/llvm-project/issues/100300 caused by yet another patch https://github.com/llvm/llvm-project/pull/96801, which in turn was an attempt at fixing a real bug https://github.com/llvm/llvm-project/issues/81385.

I totally get that. The point I am trying to make is, these sequences of patches, fixed a real regression, at the cost of introducing another one.

> In your https://github.com/llvm/llvm-project/pull/108671#discussion_r1762725421 above, it doesn't seem that your C++ example return FixedInt<N | M>(foo); is from an existing codebase.

Sorry if I gave that impression, but I was delibaretly trying to make it more concrete as you were asking for more specifics, here's some real world examples: https://github.com/search?q=lang%3Ac%2B%2B+%2FFixedInt%3C.*%5C%7C.*%3E%2F&type=code

> The fact that it happened to get formatted that way before doesn't necessarily mean that we should always format another similar pattern e.g. return Foo < A || B > (C ^ D); the same way, especially if the latter is much more likely.

No I totally get that. That's how clang-format evolves over time. But as I explained above, my understanding is this is done in a way that it doesn't regress behavior for already formatted code. As otherwise it becomes really troublesome to adopt new versions of clang-format.
Hence the change in https://github.com/llvm/llvm-project/pull/100980, didn't simply improve formatting for some mishandled pattern, it also regressed handling of some existing pattern.

I am totally OK if your verdict here is this is a cost we're willing to pay, in the end you're the maintainer. I am just trying to make sure this doesn't go unnoticed and someone is making a call deliberately. Even if it isn't aligned with the outcome that I might like.

> I've added an option in https://github.com/llvm/llvm-project/pull/109916 to help disambiguate the < in the C++ examples above.

It might be a nice addition, but it doesn't really change the fact that clang-format-18 is WAI for such code patterns, and starting with clang-format-19 this has regressed and users need to change their configuration. Moreover it also means that patch now needs to be backported (or clang-format-19 is just broken for certain codebases).

https://github.com/llvm/llvm-project/pull/108671