Page MenuHomePhabricator

[clang-format] Constructor initializer lists format with pp directives
ClosedPublic

Authored by guitard0g on Sep 17 2021, 12:25 AM.

Details

Summary

Currently constructor initializer lists sometimes format incorrectly
when there is a preprocessor directive in the middle of the list.
This patch fixes the issue when parsing the initilizer list by
ignoring the preprocessor directive when checking if a block is
part of an initializer list.

rdar://82554274

Diff Detail

Event Timeline

guitard0g requested review of this revision.Sep 17 2021, 12:25 AM
guitard0g created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2021, 12:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added a project: Restricted Project.

was there a bug report against this? what was it doing before?

@MyDeveloperDay This is an issue a coworker pointed out to me. Previously this code:

Foo::Foo(int x)
    : _x { x }
#if DEBUG
    , _y { 0 }
#endif
{
}

would format into the following:

Foo::Foo(int x)
    : _x
{
    x
}
#if DEBUG
, _y
{
    0
}
#endif
{
}

whereas this code without the preprocessor directive:

Foo::Foo(int x)
    : _x { x }
    , _y { 0 }
{
}

Is properly formatted with no changes made by clang-format.

Removing useless line from test.

guitard0g updated this revision to Diff 373167.Sep 17 2021, 1:40 AM

Fix failing tests.

MyDeveloperDay added a reviewer: owenpan.

I see, let me add some other prominent reviewers

I do see what you mean

SomeClass::Constructor()
    : x {
  x
}
#if DEBUG
, y { 0 }
#endif
{}

SomeClass::Constructor()
    : x{x}
    , y{0} {}
clang/unittests/Format/FormatTest.cpp
19329

I think its good to add what you expect without the CONDITION too!

MyDeveloperDay added inline comments.Sep 17 2021, 4:47 AM
clang/unittests/Format/FormatTest.cpp
19329

Can you add doubly nested test

SomeClass::Constructor()
    : x {
  x
}
#if WINDOWS
#if DEBUG
, y { 0 }
#else
, y { 1 }
#endif
#else
#if DEBUG
, y { 2 }
#else
, y { 3 }
#endif
#endif
{}
clang/unittests/Format/FormatTest.cpp
19301

From the name I would expect also to check

SomeClass::SomeClass()
  : a{a},
    b{b}
...

With and without the PP directive.

guitard0g updated this revision to Diff 373351.Sep 17 2021, 3:07 PM

Add test case suggestions from reviewers.

When looking at test case suggestions, I happened upon another problem that occurs when handling preprocessor directives. The following code is properly formatted (following llvm style, with ColumnLimit=0):

SomeClass::SomeClass()
  : a{a},
    b{b} {}

However this code:

Foo::Foo()
    : x{x},
#if DEBUG
      y{y},
#endif
      z{z} {}

Is transformed by clang-format (under the same style guidelines) into this:

Foo::Foo()
    : x{x},
#if DEBUG
      y{y},
#endif
      z{z} {
}

It seems there is some wonkiness with how it handles preprocessor statements here. I can address this in a another patch.

guitard0g updated this revision to Diff 373366.Sep 17 2021, 4:34 PM

Fix failing tests.

When looking at test case suggestions, I happened upon another problem that occurs when handling preprocessor directives. The following code is properly formatted (following llvm style, with ColumnLimit=0):

SomeClass::SomeClass()
  : a{a},
    b{b} {}

However this code:

Foo::Foo()
    : x{x},
#if DEBUG
      y{y},
#endif
      z{z} {}

Is transformed by clang-format (under the same style guidelines) into this:

Foo::Foo()
    : x{x},
#if DEBUG
      y{y},
#endif
      z{z} {
}

It seems there is some wonkiness with how it handles preprocessor statements here. I can address this in a another patch.

ColumnLimit of 0 seems to work very differently in many occasions.

clang/unittests/Format/FormatTest.cpp
19338

And now I'm curious, if there is a line between the nested PPs?

SomeClass::Constructot()
  : a{}
#if X
, b{}
#if Y
...

mark the comments as done once addressed please

guitard0g marked 3 inline comments as done.Tue, Sep 21, 9:01 AM

Add test case for space between directives.

guitard0g marked an inline comment as done.Tue, Sep 21, 10:14 AM
This revision is now accepted and ready to land.Tue, Sep 21, 12:42 PM
guitard0g edited the summary of this revision. (Show Details)Tue, Sep 21, 1:26 PM

@MyDeveloperDay @HazardyKnusperkeks I don't have commit access, could one of you commit this for me? Thanks so much for your review!

Name: Josh Learn
Email: joshua_learn@apple.com

(The test failures seem to be unrelated, but I'm fine waiting until they start passing again if necessary)

@MyDeveloperDay @HazardyKnusperkeks I don't have commit access, could one of you commit this for me? Thanks so much for your review!

Name: Josh Learn
Email: joshua_learn@apple.com

(The test failures seem to be unrelated, but I'm fine waiting until they start passing again if necessary)

Will do, but most likely not before the next weekend.

This revision was landed with ongoing or failed builds.Sat, Oct 2, 5:24 AM
This revision was automatically updated to reflect the committed changes.