Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

clang-format Incorrectly Removes Space After C Style Cast When Type Is Not a Pointer #47504

Open Quuxplusone opened 3 years ago

Quuxplusone commented 3 years ago
Bugzilla Link PR48535
Status CONFIRMED
Importance P normal
Reported by Gavin Howard (gavin.d.howard@gmail.com)
Reported on 2020-12-16 16:06:14 -0800
Last modified on 2020-12-21 03:52:47 -0800
Version 11.0
Hardware PC Linux
CC htmldeveloper@gmail.com, llvm-bugs@lists.llvm.org, mydeveloperday@gmail.com
Fixed by commit(s)
Attachments .clang-format (3454 bytes, text/plain)
Blocks
Blocked by
See also
Created attachment 24295
Style file

When I set the option "SpaceAfterCStyleCast" to true, clang-format will remove
a space after a C-style cast when the type inside the cast parentheses is not a
pointer time.

You can reproduce this by using the attached style file on
https://git.yzena.com/Yzena/Yc/src/branch/master/tests/fs/fs_read.c, lines 69
and 107.

If I add an asterisk after the size_t inside the parens, the space is left.

I speculate that clang-format incorrectly thinks that both sets of parens are
calls, i.e., that the first set is a call to a function that returns a function
pointer and that the second set is a call to that supposed returned function
pointer.
Quuxplusone commented 3 years ago

Attached .clang-format (3454 bytes, text/plain): Style file

Quuxplusone commented 3 years ago

This site is blocked for me, please put a simplified use case and .clang-format file into the issue (as text) not attachment

Remove all options from .clang-format that don't impact the issue being present or not

Quuxplusone commented 3 years ago

With the following saved as clang-format-c-cast.c:

nt main(int argc, char* argv[])
{
    size_t idx = (size_t) (ptr - ((char*) file));
    idx = (size_t) (ptr - ((char*) file));

    return 0;
}

Run the following command:

clang-format --style="{SpaceAfterCStyleCast: true }" clang-format-c-cast.c

Notice that the casts to size_t lose their spaces and the casts to char* do not.

Quuxplusone commented 3 years ago
Breaking it down to the simplest form

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

I believe this shows what you mean

In the format the ) of ") a" is seen as a CastRParen

 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=identifier L=6 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x1c05770 Text='size_t'
 M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x1c05790 Text='idx'
 M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=22 Name=equal L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=22 Name=l_paren L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=59 Name=identifier L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x1c05770 Text='size_t'
 M=0 C=0 T=CastRParen S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=120 Name=identifier L=23 PPK=2 FakeLParens= FakeRParens=1 II=0x1c057b0 Text='a'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=24 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

In the later its not:

 M=0 C=0 T=Unknown S=1 F=0 B=0 BK=0 P=0 Name=identifier L=6 PPK=2 FakeLParens=2/ FakeRParens=0 II=0x1c05770 Text='size_t'
 M=0 C=1 T=StartOfName S=1 F=0 B=0 BK=0 P=220 Name=identifier L=10 PPK=2 FakeLParens= FakeRParens=0 II=0x1c05790 Text='idx'
 M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=22 Name=equal L=12 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='='
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=22 Name=l_paren L=14 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=59 Name=identifier L=20 PPK=2 FakeLParens= FakeRParens=0 II=0x1c05770 Text='size_t'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=21 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=l_paren L=22 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='('
 M=0 C=1 T=Unknown S=0 F=0 B=0 BK=0 P=59 Name=identifier L=23 PPK=2 FakeLParens=13/ FakeRParens=0 II=0x1c057b0 Text='a'
 M=0 C=0 T=BinaryOperator S=1 F=0 B=0 BK=0 P=53 Name=minus L=25 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text='-'
 M=0 C=1 T=Unknown S=1 F=0 B=0 BK=0 P=53 Name=numeric_constant L=27 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text='1'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=43 Name=r_paren L=28 PPK=2 FakeLParens= FakeRParens=1 II=0x0 Text=')'
 M=0 C=0 T=Unknown S=0 F=0 B=0 BK=0 P=23 Name=semi L=29 PPK=2 FakeLParens= FakeRParens=0 II=0x0 Text=';'

As with your other issue, its not an exact science how "CastRParen" get set or
not set.

If I naive set the following in rParenEndsCast()

    if (Tok.Next->is(tok::l_paren))
      return true;

So that whenever we see ")(" we treat the first as a CastRParen, then all the
unit test pass except those that are handling a function ptr

(a->*foo)(arguments)

which would not get formatted as

(a->*foo) (arguments)

Its possible something more like

+    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;
+

Could actually help, resulting in

size_t idx = (size_t) a;
size_t idx = (size_t) (a - 1);
size_t idx = (a->*b)(a - 1);
size_t idx = (a->foo)(a - 1);
size_t idx = (*foo)(a - 1);
Quuxplusone commented 3 years ago

Sounds good. What do I need to do to help?

Quuxplusone commented 3 years ago

Tenative fix https://reviews.llvm.org/D93626