Page MenuHomePhabricator

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

Authored by MyDeveloperDay on Dec 21 2020, 3:48 AM.

Details

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

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Dec 21 2020, 3:48 AM
MyDeveloperDay created this revision.
HazardyKnusperkeks added inline comments.
clang/lib/Format/TokenAnnotator.cpp
1976

What's the policy towards unrelated changes?

MyDeveloperDay marked an inline comment as done.Dec 22 2020, 4:35 AM
MyDeveloperDay added inline comments.
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 marked an inline comment as done.

remove unrelated change

curdeius accepted this revision.Dec 22 2020, 4:49 AM

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.

This revision is now accepted and ready to land.Dec 22 2020, 4:49 AM

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) {

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!