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

[guitard0g]

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

[MyDeveloperDay]

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

[guitard0g]

@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.

[guitard0g]

Removing useless line from test.

[guitard0g]

Fix failing tests.

[MyDeveloperDay]

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

19299

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

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

19299

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

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

19271

From the name I would expect also to check

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

With and without the PP directive.

[guitard0g]

Add test case suggestions from reviewers.

[guitard0g]

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]

Fix failing tests.

[HazardyKnusperkeks]

In D109951#3007425, @guitard0g wrote:

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

19308

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

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

[MyDeveloperDay]

mark the comments as done once addressed please

[guitard0g]

Add test case for space between directives.

[HazardyKnusperkeks]

Looks good.

[MyDeveloperDay]

LGTM

[guitard0g]

@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: [e-mail]

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

[HazardyKnusperkeks]

In D109951#3015841, @guitard0g wrote:

@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: [e-mail]

(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.

clang/lib/Format/UnwrappedLineParser.cpp

do {
     case tok::r_brace:
       if (LBraceStack.empty())
         break;
       if (LBraceStack.back()->is(BK_Unknown)) {
         bool ProbablyBracedList = false;
         if (Style.Language == FormatStyle::LK_Proto) {
           ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
         } else {
+          // Skip NextTok over preprocessor lines, otherwise we may not
+          // properly diagnose the block as a braced intializer
+          // if the comma separator appears after the pp directive.
+          if (NextTok->is(tok::hash)) {
+            ScopedMacroState MacroState(*Line, Tokens, NextTok);
+            do {
+              NextTok = Tokens->getNextToken();
+              ++ReadTokens;
+            } while (NextTok->isNot(tok::eof));
+          }
+
           // Using OriginalColumn to distinguish between ObjC methods and
           // binary operators is a bit hacky.
           bool NextIsObjCMethod = NextTok->isOneOf(tok::plus, tok::minus) &&
                                   NextTok->OriginalColumn == 0;
 
           // If there is a comma, semicolon or right paren after the closing
           // brace, we assume this is a braced initializer list.  Note that
           // regardless how we mark inner braces here, we will overwrite the

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, BreakConstructorInitializersBeforeComma) {
   Style.ColumnLimit = 60;
   verifyFormat("SomeClass::Constructor()\n"
                "    : aaaaaaaa(aaaaaaaa)\n"
                "    , aaaaaaaa(aaaaaaaa)\n"
                "    , aaaaaaaa(aaaaaaaa) {}",
                Style);
 }
 
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+               "    : a{a}\n"
+               "#if CONDITION\n"
+               "    , b{b}\n"
+               "#endif\n"
+               "{\n}",
+               Style);
+  Style.ConstructorInitializerIndentWidth = 2;
+  verifyFormat("SomeClass::Constructor()\n"
+               "#if CONDITION\n"
+               "  : a{a}\n"
+               "#endif\n"
+               "  , b{b}\n"
+               "  , c{c} {\n}",
+               Style);
+  Style.ConstructorInitializerIndentWidth = 0;
+  verifyFormat("SomeClass::Constructor()\n"
+               ": a{a}\n"
+               "#ifdef CONDITION\n"
+               ", b{b}\n"
+               "#else\n"
+               ", c{c}\n"
+               "#endif\n"
+               ", d{d} {\n}",
+               Style);
+}
+
 TEST_F(FormatTest, Destructors) {
   verifyFormat("void F(int &i) { i.~int(); }");
   verifyFormat("void F(int &i) { i->~int(); }");
 }
 
 TEST_F(FormatTest, FormatsWithWebKitStyle) {
   FormatStyle Style = getWebKitStyle();
 
   // Don't indent in outer namespaces.
   verifyFormat("namespace outer {\n"
                "int i;\n"
                "namespace inner {\n"
                "    int i;\n"
                "} // namespace inner\n"
                "} // namespace outer\n"