[clang-format] Refactor determineStarAmpUsage NFC

[sstwcw]

Summary

There was some duplicate code in determineStarAmpUsage and
determinePlusMinusCaretUsage

Now a - or + following ;, sizeof, co_await, or delete is
regarded as a unary operator.

Now a * or & following case is also a unary operator.

[curdeius]

clang/lib/Format/TokenAnnotator.cpp

2202–2205

As below, before, question, kw_return, kw_case, at were not handled here.
Please add tests.

2255–2256

I don't like this part of your patch. You remove comments for no real gain.

2256–2260

There were no TT_ConditionalExpr, semi, kw_co_await, kw_delete, kw_sizeof, kw_throw before.
I'd like to see tests for each of them.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

2202–2205

This needs a comment or better a new common function. Otherwise it's just unnecessary hard to understand why we now check for PlusMinusCaret. (I've never heard of this function and I've stepped a lot through clang-format. ;))

[sstwcw]

Add some test cases and use a separate function for the common parts.

[sstwcw]

About the tokens that were only in one function.

question, colon, and TT_ConditionalExpr, are for same thing. question was added before they added TT_ConditionalExpr. It looks like now only TT_ConditionalExpr would be enough.

kw_return and kw_throw were in both functions.

kw_case, kw_co_await, and kw_delete are now only in one function like before this revisioin.

semi and kw_sizeof have test cases.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

2138

I'm not sure about this. Why not handle them here too?

2146–2147

Especially here, why should a + after delete be a binary operator?
How much sense it makes doesn't matter, it is valid c++: https://gcc.godbolt.org/z/c1x1nn3Ej

clang/unittests/Format/FormatTest.cpp

9758

A format test is fine, a token annotator test would be better.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

2146–2147

I'm also trying to understand did you mean you couldn't have

case -1:
case +1:
case +0:
case -0:

https://gcc.godbolt.org/z/qvE44d5xz

[sstwcw]

clang/lib/Format/TokenAnnotator.cpp

2146–2147

In the new version + following delete is a unary operator. Previously I was under the impression that we only formatted code that is sensible.

2146–2147

case -1:
case +1:

I meant we couldn't have things like case *x or case &x. Is the comment not clear enough?

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

2146–2147

case -1:
case +1:

I meant we couldn't have things like case *x or case &x. Is the comment not clear enough?

But we can: https://gcc.godbolt.org/z/8Mb1xo7oP

In the new version + following delete is a unary operator. Previously I was under the impression that we only formatted code that is sensible.

What is sensible this is rather subjective, isn't it? I went a long way to get requires clauses and expressions doing to "right thing" and even added explicitly the unit tests for stuff I think no sane person would ever write, but it's valid code. So in my opinion it should be formatted correctly. I got it wrong on a couple of cases, all but one that I'm aware of are fixed now.

And even if it were invalid code, when it makes the functions to reason about easier why not? I think invalid code can be formatted weirdly, once it's valid again we will format it correctly. But all valid code should be formatted as best as we can.

2146–2147

In the new version + following delete is a unary operator. Previously I was under the impression that we only formatted code that is sensible.

clang/unittests/Format/TokenAnnotatorTest.cpp

85 ↗

(On Diff #416450)

I know the other cases also use EXPECT, but the size should be an ASSERT, so there would be no seg fault if the number would change (it shouldn't).

157 ↗

(On Diff #416450)

This is the Cast Paren case, right? Please also add the paren check.

162 ↗

(On Diff #416450)

The exclaim too?

[MyDeveloperDay]

But we can: https://gcc.godbolt.org/z/8Mb1xo7oP

This is what I love about you all, you push me to learn more about C++, its a slippery b****r

[sstwcw]

clang/unittests/Format/TokenAnnotatorTest.cpp

85 ↗

(On Diff #416450)

I tried a wrong number and EXPECT_EQ didn't give a segfault.

[HazardyKnusperkeks]

Just a side note, I often get this on your changes:
Unhandled Exception ("Exception")
Found unknown intradiff source line, expected a line beginning with "+", "-", or " " (space): \ No newline at end of file
.

clang/unittests/Format/TokenAnnotatorTest.cpp

85 ↗

(On Diff #416450)

The potential segfault is not in EXPECT_EQ, but in accessing a non existent token.
The number of tokens will most likely never change, but than again we don't need the EXPECT_EQ either.
But if it changes (gets smaller) we want to abort the test after the failed check and not accessing Tokens with an invalid index, that's why I'm pleading (and using) ASSERT_EQ.

[HazardyKnusperkeks]

I cannot view the diff between the last two revisions:

Unhandled Exception ("Exception")
Found unknown intradiff source line, expected a line beginning with "+", "-", or " " (space): \ No newline at end of file
.

[sstwcw]

Maybe it's because I removed the final empty line when I pasted the diff as I thought that LF was a line terminator instead of a line separator. I will try using arc from now on.

clang/lib/Format/TokenAnnotator.cpp

bool rParenEndsCast(const FormatToken &Tok) {
     // Search for unexpected tokens.
     for (FormatToken *Prev = Tok.Previous; Prev != Tok.MatchingParen;
          Prev = Prev->Previous)
       if (!Prev->isOneOf(tok::kw_const, tok::identifier, tok::coloncolon))
         return false;
     return true;
   }
 
+  /// Returns true if the token is used as a unary operator.
+  bool determineUnaryOperatorByUsage(const FormatToken &Tok) {
+    const FormatToken *PrevToken = Tok.getPreviousNonComment();
+    if (!PrevToken)
+      return true;
+
+    // These keywords are deliberately not included here. They are either
+    // included in determineStarAmpUsage or determinePlusMinusCaretUsage.
+    // case - In a switch expression, the condition shall be of integral type,
+    //   enumeration type, or class type, and the case label contains a constant
+    //   expression. We were unable to come up with a case where the case
+    //   keyword is followed by a star or amp.
+    // @ - It may be followed by a unary `-` in Objective-C literals. We don't
+    //   know how they can be followed by a star or amp.
+    // co_await, delete - It doesn't make sense to have them followed by a unary
+    //   `+` or `-`.
+    if (PrevToken->isOneOf(TT_ConditionalExpr, tok::l_paren, tok::comma,
+                           tok::colon, tok::semi, tok::equal, tok::question,
+                           tok::l_square, tok::l_brace, tok::kw_co_return,
+                           tok::kw_co_yield, tok::kw_return, tok::kw_throw))
+      return true;
+
+    // We put sizeof here instead of only in determineStarAmpUsage. In the cases
+    // where the unary `+` operator is overloaded, it is reasonable to write
+    // things like `sizeof +x`. Like commit 446d6ec996c6c3.
+    if (PrevToken->is(tok::kw_sizeof))
+      return true;
+
+    // A sequence of leading unary operators.
+    if (PrevToken->isOneOf(TT_CastRParen, TT_UnaryOperator))
+      return true;
+
+    // There can't be two consecutive binary operators.
+    if (PrevToken->is(TT_BinaryOperator))
+      return true;
+
+    return false;
+  }
+
   /// Return the type of the given token assuming it is * or &.
   TokenType determineStarAmpUsage(const FormatToken &Tok, bool IsExpression,
                                   bool InTemplateArgument) {
     if (Style.isJavaScript())
       return TT_BinaryOperator;
 
     // && in C# must be a binary operator.
     if (Style.isCSharp() && Tok.is(tok::ampamp))
if (!NextToken ||
       return TT_PointerOrReference;
 
     if (PrevToken->is(tok::coloncolon))
       return TT_PointerOrReference;
 
     if (PrevToken->is(tok::r_paren) && PrevToken->is(TT_TypeDeclarationParen))
       return TT_PointerOrReference;
 
-    if (PrevToken->isOneOf(tok::l_paren, tok::l_square, tok::l_brace,
-                           tok::comma, tok::semi, tok::kw_return, tok::colon,
-                           tok::kw_co_return, tok::kw_co_await,
-                           tok::kw_co_yield, tok::equal, tok::kw_delete,
-                           tok::kw_sizeof, tok::kw_throw, TT_BinaryOperator,
-                           TT_ConditionalExpr, TT_UnaryOperator, TT_CastRParen))
+    if (determineUnaryOperatorByUsage(Tok))
+      return TT_UnaryOperator;
+
+    if (PrevToken->isOneOf(tok::kw_co_await, tok::kw_delete))
       return TT_UnaryOperator;
 
     if (NextToken->is(tok::l_square) && NextToken->isNot(TT_LambdaLSquare))
       return TT_PointerOrReference;
     if (NextToken->is(tok::kw_operator) && !IsExpression)
       return TT_PointerOrReference;
     if (NextToken->isOneOf(tok::comma, tok::semi))
       return TT_PointerOrReference;
TokenType determineStarAmpUsage(const FormatToken &Tok, bool IsExpression,
     // definition on the RHS of an assignment.
     if (IsExpression && !Contexts.back().CaretFound)
       return TT_BinaryOperator;
 
     return TT_PointerOrReference;
   }
 
   TokenType determinePlusMinusCaretUsage(const FormatToken &Tok) {
-    const FormatToken *PrevToken = Tok.getPreviousNonComment();
-    if (!PrevToken)
-      return TT_UnaryOperator;
-
-    if (PrevToken->isOneOf(TT_CastRParen, TT_UnaryOperator))
-      // This must be a sequence of leading unary operators.
+    if (determineUnaryOperatorByUsage(Tok))
       return TT_UnaryOperator;
 
-    // Use heuristics to recognize unary operators.
-    if (PrevToken->isOneOf(tok::equal, tok::l_paren, tok::comma, tok::l_square,
-                           tok::question, tok::colon, tok::kw_return,
-                           tok::kw_case, tok::at, tok::l_brace, tok::kw_throw,
-                           tok::kw_co_return, tok::kw_co_yield))
+    const FormatToken *PrevToken = Tok.getPreviousNonComment();
+    if (!PrevToken)
       return TT_UnaryOperator;
 
-    // There can't be two consecutive binary operators.
-    if (PrevToken->is(TT_BinaryOperator))
+    if (PrevToken->isOneOf(tok::at, tok::kw_case))
       return TT_UnaryOperator;
 
     // Fall back to marking the token as binary operator.
     return TT_BinaryOperator;
   }
 
   /// Determine whether ++/-- are pre- or post-increments/-decrements.
   TokenType determineIncrementUsage(const FormatToken &Tok) {
     const FormatToken *PrevToken = Tok.getPreviousNonComment();
     if (!PrevToken || PrevToken->is(TT_CastRParen))
       return TT_UnaryOperator;
     if (PrevToken->isOneOf(tok::r_paren, tok::r_square, tok::identifier))
       return TT_TrailingUnaryOperator;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsUnaryOperators) {
   verifyFormat("if (i < -1) {\n}");
   verifyFormat("++(a->f());");
   verifyFormat("--(a->f());");
   verifyFormat("(a->f())++;");
   verifyFormat("a[42]++;");
   verifyFormat("if (!(a->f())) {\n}");
   verifyFormat("if (!+i) {\n}");
   verifyFormat("~&a;");
+  verifyFormat("for (x = 0; -10 < x; --x) {\n}");
+  verifyFormat("sizeof -x");
+  verifyFormat("sizeof +x");
 
   verifyFormat("a-- > b;");
   verifyFormat("b ? -a : c;");
   verifyFormat("n * sizeof char16;");
   verifyFormat("n * alignof char16;", getGoogleStyle());
   verifyFormat("sizeof(char);");
   verifyFormat("alignof(char);", getGoogleStyle());