Page MenuHomePhabricator

[clang-format] PR50326 AlignAfterOpenBracket AlwaysBreak does not keep to the ColumnLimit
ClosedPublic

Authored by MyDeveloperDay on May 13 2021, 3:42 AM.

Details

Summary

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

D93626: [clang-format] PR48535 clang-format Incorrectly Removes Space After C Style Cast When Type Is Not a Pointer caused a regression in terms of formatting a function ptr, incorrectly thinking it was a C-Style cast.

This cased a formatter regression between clang-format-11 and clang-format-12

void bar()
{
    size_t foo = function(Foooo, Barrrrr, Foooo, Barrrr, FoooooooooLooooong);

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

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

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

became

void bar()
{
    size_t foo1 = function(Foooo, Barrrrr, Foooo, Barrrr, FoooooooooLooooong);

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

    size_t foo3 = (*(function))(Foooo, Barrrrr, Foooo, FoooooooooLooooong);

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

This fixes this issue by simplify the clause to be specific about what is wanted rather than what is not.

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.May 13 2021, 3:42 AM
MyDeveloperDay created this revision.
curdeius accepted this revision.May 13 2021, 6:45 AM

LGTM. Not blocking, but I'd add foo4 example from the description as a test case (with a link to the bug maybe).

This revision is now accepted and ready to land.May 13 2021, 6:45 AM

I second that ColumnLimit breaking test case. :)

LGTM. Not blocking, but I'd add foo4 example from the description as a test case (with a link to the bug maybe).

Of course I should have added that too! let me do that!

Add a test that shows it no longer impacts function ptrs

slightly closer test to the original failure

This revision was landed with ongoing or failed builds.May 15 2021, 3:31 AM
This revision was automatically updated to reflect the committed changes.
MyDeveloperDay added a comment.EditedMay 15 2021, 4:10 AM

This is also the cause of https://bugs.llvm.org/show_bug.cgi?id=49995, we might want to ask for this to be put into any 12.0.1 patch, I'm not sure who we need to speak to about getting something put into a patch @benhamilton, @probinson