[clang-format] PR48535 clang-format Incorrectly Removes Space After C Style Cast When Type Is Not a Pointer

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=48535

using SpaceAfterCStyleCast: true

size_t idx = (size_t) a;
size_t idx = (size_t) (a - 1);

is formatted as:

size_t idx = (size_t) a;
size_t idx = (size_t)(a - 1);

This revision aims to improve that by improving the function which tries to identify a CastRParen

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

1976

What's the policy towards unrelated changes?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

1976

ideally not, but we try to keep clang-format, clang-formatted.

I'll remove from the patch and apply separately.

[MyDeveloperDay]

remove unrelated change

[curdeius]

LGTM now. I tried to find other cases where this change may change the behaviour but couldn't. Have you tried applying to some bigger repo and see what you get? The best would be a repo with SpaceAfterCStyleCast=true.

[MyDeveloperDay]

In D93626#2467745, @curdeius wrote:

LGTM now. I tried to find other cases where this change may change the behaviour but couldn't. Have you tried applying to some bigger repo and see what you get? The best would be a repo with SpaceAfterCStyleCast=true.

Only my own internally, which I preconverted to using this style using an older clang-format then ran with this version. This seems to be fixing more cases than creating false positives:

Here are just 2 additional cases I found

int a = (int16)(a + (b << 2) + (c << 4));
if ((size_t)(a - b) <= c)

which become

int a = (int16) (a + (b << 2) + (c << 4));
if ((size_t) (a - b) <= c) {

[curdeius]

In D93626#2467927, @MyDeveloperDay wrote:

In D93626#2467745, @curdeius wrote:

LGTM now. I tried to find other cases where this change may change the behaviour but couldn't. Have you tried applying to some bigger repo and see what you get? The best would be a repo with SpaceAfterCStyleCast=true.

Only my own internally, which I preconverted to using this style using an older clang-format then ran with this version. This seems to be fixing more cases than creating false positives:

Here are just 2 additional cases I found

int a = (int16)(a + (b << 2) + (c << 4));
if ((size_t)(a - b) <= c)

which become

int a = (int16) (a + (b << 2) + (c << 4));
if ((size_t) (a - b) <= c) {

Great!

[MyDeveloperDay]

This fix seems to cause a regression. (https://bugs.llvm.org/show_bug.cgi?id=50326) causing ColumLimit to not be observed in the following example

size_t foo = (*(function))(
    Foooo, Barrrrr, Foooo, Barrrr, FoooooooooLooooong, BarrrrrrrrrrrrLong,
    FoooooooooLooooong);

becomes

  size_t foo = (*(
      function))(Foooo, Barrrrr, Foooo, Barrrr, FoooooooooLooooong, Barrrrrrrrrr
rrLong, FoooooooooLooooong);

after this change

clang/lib/Format/TokenAnnotator.cpp

for (const FormatToken *Token = Tok.MatchingParen->Next; Token != &Tok;
       if (Token->is(TT_BinaryOperator))
         return false;
 
     // If the following token is an identifier or 'this', this is a cast. All
     // cases where this can be something else are handled above.
     if (Tok.Next->isOneOf(tok::identifier, tok::kw_this))
       return true;
 
+    if (Tok.Next->is(tok::l_paren) &&
+        !(Tok.Previous && Tok.Previous->is(tok::identifier) &&
+          Tok.Previous->Previous &&
+          Tok.Previous->Previous->isOneOf(tok::arrowstar, tok::arrow,
+                                          tok::star)))
+      return true;
+
     if (!Tok.Next->Next)
       return false;
 
     // If the next token after the parenthesis is a unary operator, assume
     // that this is cast, unless there are unexpected tokens inside the
     // parenthesis.
     bool NextIsUnary =
         Tok.Next->isUnaryOperator() || Tok.Next->isOneOf(tok::amp, tok::star);
TokenType determineStarAmpUsage(const FormatToken &Tok, bool IsExpression,
     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) ||
+                           tok::kw_co_return, tok::kw_co_await,
+                           tok::kw_co_yield, tok::equal, tok::kw_delete,
+                           tok::kw_sizeof, tok::kw_throw) ||
         PrevToken->isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
                            TT_UnaryOperator, TT_CastRParen))
       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;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
   Spaces.SpaceAfterCStyleCast = true;
   verifyFormat("while ((bool) 1)\n"
                "  continue;",
                Spaces);
   verifyFormat("do {\n"
                "  do_something((int) i);\n"
                "} while (something( ));",
                Spaces);
+
+  verifyFormat("size_t idx = (size_t) (ptr - ((char *) file));", Spaces);
+  verifyFormat("size_t idx = (size_t) a;", Spaces);
+  verifyFormat("size_t idx = (size_t) (a - 1);", Spaces);
+  verifyFormat("size_t idx = (a->*foo)(a - 1);", Spaces);
+  verifyFormat("size_t idx = (a->foo)(a - 1);", Spaces);
+  verifyFormat("size_t idx = (*foo)(a - 1);", Spaces);
+  Spaces.SpaceAfterCStyleCast = false;
+  verifyFormat("size_t idx = (size_t)(ptr - ((char *)file));", Spaces);
+  verifyFormat("size_t idx = (size_t)a;", Spaces);
+  verifyFormat("size_t idx = (size_t)(a - 1);", Spaces);
+  verifyFormat("size_t idx = (a->*foo)(a - 1);", Spaces);
+  verifyFormat("size_t idx = (a->foo)(a - 1);", Spaces);
+  verifyFormat("size_t idx = (*foo)(a - 1);", Spaces);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInSquareBrackets) {
   verifyFormat("int a[5];");
   verifyFormat("a[3] += 42;");
 
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpacesInSquareBrackets = true;