subject:"\[PATCH\] D109951\: \[clang\-format\] Constructor initializer lists format with pp directives"

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-10-02 Thread MyDeveloperDay via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3d209c76ddb5: [clang-format] Constructor initializer lists 
format with pp directives (authored by guitard0g, committed by MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19298,6 +19298,78 @@
Style);
 }
 
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   ", b{b} {}",
+   Style);
+  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);
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   "#if WINDOWS\n"
+   "#if DEBUG\n"
+   ", b{0}\n"
+   "#else\n"
+   ", b{1}\n"
+   "#endif\n"
+   "#else\n"
+   "#if DEBUG\n"
+   ", b{2}\n"
+   "#else\n"
+   ", b{3}\n"
+   "#endif\n"
+   "#endif\n"
+   "{\n}",
+   Style);
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   "#if WINDOWS\n"
+   ", b{0}\n"
+   "#if DEBUG\n"
+   ", c{0}\n"
+   "#else\n"
+   ", c{1}\n"
+   "#endif\n"
+   "#else\n"
+   "#if DEBUG\n"
+   ", c{2}\n"
+   "#else\n"
+   ", c{3}\n"
+   "#endif\n"
+   ", b{1}\n"
+   "#endif\n"
+   "{\n}",
+   Style);
+}
+
 TEST_F(FormatTest, Destructors) {
   verifyFormat("void F(int ) { i.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
 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.
+  while (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) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-27 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-22 Thread Josh Learn via Phabricator via cfe-commits

guitard0g added a comment.

@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_le...@apple.com

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-22 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-21 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.
This revision is now accepted and ready to land.

Looks good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-21 Thread Josh Learn via Phabricator via cfe-commits

guitard0g updated this revision to Diff 373977.
guitard0g added a comment.

Add test case for space between directives.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,78 @@
Style);
 }
 
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   ", b{b} {}",
+   Style);
+  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);
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   "#if WINDOWS\n"
+   "#if DEBUG\n"
+   ", b{0}\n"
+   "#else\n"
+   ", b{1}\n"
+   "#endif\n"
+   "#else\n"
+   "#if DEBUG\n"
+   ", b{2}\n"
+   "#else\n"
+   ", b{3}\n"
+   "#endif\n"
+   "#endif\n"
+   "{\n}",
+   Style);
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   "#if WINDOWS\n"
+   ", b{0}\n"
+   "#if DEBUG\n"
+   ", c{0}\n"
+   "#else\n"
+   ", c{1}\n"
+   "#endif\n"
+   "#else\n"
+   "#if DEBUG\n"
+   ", c{2}\n"
+   "#else\n"
+   ", c{3}\n"
+   "#endif\n"
+   ", b{1}\n"
+   "#endif\n"
+   "{\n}",
+   Style);
+}
+
 TEST_F(FormatTest, Destructors) {
   verifyFormat("void F(int ) { i.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
 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.
+  while (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) &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-21 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added a comment.

mark the comments as done once addressed please


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-18 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

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.




Comment at: clang/unittests/Format/FormatTest.cpp:19308
+   "#if WINDOWS\n"
+   "#if DEBUG\n"
+   ", b{0}\n"

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

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread Josh Learn via Phabricator via cfe-commits

guitard0g updated this revision to Diff 373366.
guitard0g added a comment.

Fix failing tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,59 @@
Style);
 }
 
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   ", b{b} {}",
+   Style);
+  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);
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   "#if WINDOWS\n"
+   "#if DEBUG\n"
+   ", b{0}\n"
+   "#else\n"
+   ", b{1}\n"
+   "#endif\n"
+   "#else\n"
+   "#if DEBUG\n"
+   ", b{2}\n"
+   "#else\n"
+   ", b{3}\n"
+   "#endif\n"
+   "#endif\n"
+   "{\n}",
+   Style);
+}
+
 TEST_F(FormatTest, Destructors) {
   verifyFormat("void F(int ) { i.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
 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.
+  while (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) &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,59 @@
Style);
 }
 
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   ", b{b} {}",
+   Style);
+  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);
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread Josh Learn via Phabricator via cfe-commits

guitard0g added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread Josh Learn via Phabricator via cfe-commits

guitard0g updated this revision to Diff 373351.
guitard0g added a comment.

Add test case suggestions from reviewers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,59 @@
Style);
 }
 
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   ", b{b} {}",
+   Style);
+  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);
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   "#if WINDOWS\n"
+   "#if DEBUG\n"
+   ", b{0}\n"
+   "#else\n"
+   ", b{1}\n"
+   "#endif\n"
+   "#else\n"
+   "#if DEBUG\n"
+   ", b{2}\n"
+   "#else\n"
+   ", b{3}\n"
+   "#endif\n"
+   "#endif\n"
+   "{\n}",
+   Style);
+}
+
 TEST_F(FormatTest, Destructors) {
   verifyFormat("void F(int ) { i.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
 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) &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,59 @@
Style);
 }
 
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+  FormatStyle Style = getLLVMStyle();
+  Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
+  Style.ConstructorInitializerIndentWidth = 4;
+  verifyFormat("SomeClass::Constructor()\n"
+   ": a{a}\n"
+   ", b{b} {}",
+   Style);
+  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);
+  Style.ConstructorInitializerIndentWidth = 4;
+

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:19271
 
+TEST_F(FormatTest, ConstructorInitializersWithPreprocessorDirective) {
+  FormatStyle Style = getLLVMStyle();

From the name I would expect also to check
```SomeClass::SomeClass()
  : a{a},
b{b}
...
```
With and without the PP directive.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:19299
+   ", d{d} {\n}",
+   Style);
+}

MyDeveloperDay wrote:
> I think its good to add what you expect without the CONDITION too!
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
{}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:19299
+   ", d{d} {\n}",
+   Style);
+}

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread Josh Learn via Phabricator via cfe-commits

guitard0g updated this revision to Diff 373167.
guitard0g added a comment.

Fix failing tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,37 @@
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.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
 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) &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,37 @@
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.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -489,6 +489,17 @@
 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();
+

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread Josh Learn via Phabricator via cfe-commits

guitard0g updated this revision to Diff 373158.
guitard0g added a comment.

Removing useless line from test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,37 @@
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.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -459,6 +459,17 @@
   ++ReadTokens;
 } while (NextTok->is(tok::comment));
 
+// Skip 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));
+}
+
 switch (Tok->Tok.getKind()) {
 case tok::l_brace:
   if (Style.Language == FormatStyle::LK_JavaScript && PrevTok) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,37 @@
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.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -459,6 +459,17 @@
   ++ReadTokens;
 } while (NextTok->is(tok::comment));
 
+// Skip 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));
+}
+
 switch (Tok->Tok.getKind()) {
 case tok::l_brace:
   if (Style.Language == FormatStyle::LK_JavaScript && PrevTok) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread Josh Learn via Phabricator via cfe-commits

guitard0g added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109951/new/

https://reviews.llvm.org/D109951

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

2021-09-17 Thread Josh Learn via Phabricator via cfe-commits

guitard0g created this revision.
guitard0g requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109951

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,38 @@
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;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+  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.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -459,6 +459,17 @@
   ++ReadTokens;
 } while (NextTok->is(tok::comment));
 
+// Skip 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));
+}
+
 switch (Tok->Tok.getKind()) {
 case tok::l_brace:
   if (Style.Language == FormatStyle::LK_JavaScript && PrevTok) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -19268,6 +19268,38 @@
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;
+  Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+  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.~int(); }");
   verifyFormat("void F(int ) { i->~int(); }");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -459,6 +459,17 @@
   ++ReadTokens;
 } while (NextTok->is(tok::comment));
 
+// Skip 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 {
+

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

[PATCH] D109951: [clang-format] Constructor initializer lists format with pp directives

20 matches

Site Navigation

Mail list logo

Footer information