[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Possible bug https://bugs.llvm.org/show_bug.cgi?id=51640




Comment at: clang/include/clang/Format/Format.h:547
   ///   false:
-  ///   enum
-  ///   {
+  ///   enum {
   /// A,

Post commit Nit: @lunasorcery 

Rule of thumb in clang-format dev, if you change Format.h its very likely that 
ClangFormatStyleOptions.rst will need updating

This can be done by running clang/docs/tools/dump_format_style.py and that not 
always obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99840

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

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[Luna Kirkby via Phabricator via cfe-commits]

lunasorcery added a comment.

Looking at this again I think the change is flawed. The `InitialToken` is taken 
*after* the enum token, and additionally the invocation of 
`ShouldBreakBeforeBrace` is falling through to the `return false;` case every 
time. As such the call to `addUnwrappedLine()` is _always_ skipped. This 
removes the newline for both the Attach and Break styles - but the Break style 
is then fixed up by some code in `TokenAnnotator.cpp` that I don't fully 
understand, which adds the newline back in.

So this change _works_, but not quite for the right reasons.

D106349  appears to implement this in a 
slightly more correct manner, taking the `InitialToken` _before_ we've advanced 
past the enum token, and handling the enum case inside `ShouldBreakBeforeBrace`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99840

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

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[Marek Kurdej via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG71616722d409: [clang-format] Correctly attach enum braces 
with ShortEnums disabled (authored by lunasorcery, committed by curdeius).

Changed prior to commit:
  https://reviews.llvm.org/D99840?vs=362220&id=362302#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99840

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -402,8 +402,7 @@
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-   "{\n"
+  verifyFormat("public enum var {\n"
"none,\n"
"@string,\n"
"bool,\n"
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2451,6 +2451,14 @@
   Style.AllowShortEnumsOnASingleLine = true;
   verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
   Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum {\n"
+   "  A,\n"
+   "  B,\n"
+   "  C\n"
+   "} ShortEnum1, ShortEnum2;",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
   verifyFormat("enum\n"
"{\n"
"  A,\n"
@@ -22123,8 +22131,7 @@
Style);
   // Enumerations are not records and should be unaffected.
   Style.AllowShortEnumsOnASingleLine = false;
-  verifyFormat("enum class E\n"
-   "{\n"
+  verifyFormat("enum class E {\n"
"  A,\n"
"  B\n"
"};\n",
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2515,6 +2515,8 @@
   if (FormatTok->Tok.is(tok::kw_enum))
 nextToken();
 
+  const FormatToken &InitialToken = *FormatTok;
+
   // In TypeScript, "enum" can also be used as property name, e.g. in interface
   // declarations. An "enum" keyword followed by a colon would be a syntax
   // error and thus assume it is just an identifier.
@@ -2561,7 +2563,8 @@
 return true;
   }
 
-  if (!Style.AllowShortEnumsOnASingleLine)
+  if (!Style.AllowShortEnumsOnASingleLine &&
+  ShouldBreakBeforeBrace(Style, InitialToken))
 addUnwrappedLine();
   // Parse enum body.
   nextToken();
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -544,8 +544,7 @@
   ///   enum { A, B } myEnum;
   ///
   ///   false:
-  ///   enum
-  ///   {
+  ///   enum {
   /// A,
   /// B
   ///   } myEnum;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -153,7 +153,8 @@
 clang-format
 
 
-- ...
+- Option ``AllowShortEnumsOnASingleLine: false`` has been improved, it now
+  correctly places the opening brace according to ``BraceWrapping.AfterEnum``.
 
 libclang
 


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -402,8 +402,7 @@
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-   "{\n"
+  verifyFormat("public enum var {\n"
"none,\n"
"@string,\n"
"bool,\n"
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2451,6 +2451,14 @@
   Style.AllowShortEnumsOnASingleLine = true;
   verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
   Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum {\n"
+   "  A,\n"
+   "  B,\n"
+   "  C\n"
+   "} ShortEnum1, ShortEnum2;",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
   verifyFormat("enum\n"
"{\n"
"  A,\n"
@@ -22123,8 +22131,7 @@
Style);
   // Enumerations are 

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[Luna Kirkby via Phabricator via cfe-commits]

lunasorcery updated this revision to Diff 362220.
lunasorcery added a comment.

Sorry it took me so long to get back to this - fell off my radar somewhat.
I've updated the release notes, I think it's ready to commit now?
I lack commit rights (first time contributor!) so for attribution it'd be "Luna 
Kirkby "


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

https://reviews.llvm.org/D99840

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -402,8 +402,7 @@
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-   "{\n"
+  verifyFormat("public enum var {\n"
"none,\n"
"@string,\n"
"bool,\n"
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2451,6 +2451,14 @@
   Style.AllowShortEnumsOnASingleLine = true;
   verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
   Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum {\n"
+   "  A,\n"
+   "  B,\n"
+   "  C\n"
+   "} ShortEnum1, ShortEnum2;",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
   verifyFormat("enum\n"
"{\n"
"  A,\n"
@@ -22123,8 +22131,7 @@
Style);
   // Enumerations are not records and should be unaffected.
   Style.AllowShortEnumsOnASingleLine = false;
-  verifyFormat("enum class E\n"
-   "{\n"
+  verifyFormat("enum class E {\n"
"  A,\n"
"  B\n"
"};\n",
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2515,6 +2515,8 @@
   if (FormatTok->Tok.is(tok::kw_enum))
 nextToken();
 
+  const FormatToken &InitialToken = *FormatTok;
+
   // In TypeScript, "enum" can also be used as property name, e.g. in interface
   // declarations. An "enum" keyword followed by a colon would be a syntax
   // error and thus assume it is just an identifier.
@@ -2561,7 +2563,8 @@
 return true;
   }
 
-  if (!Style.AllowShortEnumsOnASingleLine)
+  if (!Style.AllowShortEnumsOnASingleLine &&
+  ShouldBreakBeforeBrace(Style, InitialToken))
 addUnwrappedLine();
   // Parse enum body.
   nextToken();
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -544,8 +544,7 @@
   ///   enum { A, B } myEnum;
   ///
   ///   false:
-  ///   enum
-  ///   {
+  ///   enum {
   /// A,
   /// B
   ///   } myEnum;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -284,6 +284,9 @@
 
 - Support for formatting JSON file (\*.json) has been added to clang-format.
 
+- Option ``AllowShortEnumsOnASingleLine: false`` has been improved, it now
+  correctly places the opening brace according to ``BraceWrapping.AfterEnum``.
+
 libclang
 
 


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -402,8 +402,7 @@
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-   "{\n"
+  verifyFormat("public enum var {\n"
"none,\n"
"@string,\n"
"bool,\n"
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2451,6 +2451,14 @@
   Style.AllowShortEnumsOnASingleLine = true;
   verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
   Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum {\n"
+   "  A,\n"
+   "  B,\n"
+   "  C\n"
+   "} ShortEnum1, ShortEnum2;",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
   verifyFormat("enum\n"
"{\n"
"  A,\n"
@@ -22123,8 +22131,7 @@
Style);
   // Enumerat

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.

In D99840#2718959 , @curdeius wrote:

> LGTM. I also consider it a bug. LLVM should not be affected as it uses 
> `AllowShortEnumsOnASingleLine: true` whereas this problem arises only with 
> `AllowShortEnumsOnASingleLine: false`.
> Anyway, those that find the new behaviour problematic, can always set 
> `BreakBeforeBraces: Custom` and `BraceWrapping.AfterEnum: true` to get the 
> old behaviour.
> @lunasorcery, please update release notes before landing.
> If you don't have commit rights, please provide "Your Name " 
> for commit attribution, otherwise we'll use the name/email associated with 
> you Phabricator account.

What he says.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99840

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

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[Marek Kurdej via Phabricator via cfe-commits]

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

LGTM. I also consider it a bug. LLVM should not be affected as it uses 
`AllowShortEnumsOnASingleLine: true` whereas this problem arises only with 
`AllowShortEnumsOnASingleLine: false`.
Anyway, those that find the new behaviour problematic, can always set 
`BreakBeforeBraces: Custom` and `BraceWrapping.AfterEnum: true` to get the old 
behaviour.
@lunasorcery, please update release notes before landing.
If you don't have commit rights, please provide "Your Name " for 
commit attribution, otherwise we'll use the name/email associated with you 
Phabricator account.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99840

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

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> I don't think we should change the LLVM style

I'm not 100% convinced we are changing the LLVM style per say.

LLVM always "cuddles" the "{"  and Allow short Enums on a single line is true, 
this means if the "{" is on a new line then I think its actually a bug. 
@curdeius  what say you?

It be interested to know the impact on the LLVM tree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99840

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

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

While the fix seems to be right, I don't think we should change the LLVM style 
(and possibly other styles). So my suggestion is to apply the fix, but actually 
change the style so that it behaves like before, so that braces of enums are 
wrapped.

And maybe add an entry in the changelog.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99840

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

[PATCH] D99840: [clang-format] Correctly attach enum braces with ShortEnums disabled

[Luna Kirkby via Phabricator via cfe-commits]

lunasorcery created this revision.
lunasorcery added reviewers: djasper, klimek.
lunasorcery added a project: clang-format.
lunasorcery requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Previously, with AllowShortEnumsOnASingleLine disabled, enums that would have 
otherwise fit on a single line would always put the opening brace on its own 
line.
This patch ensures that these enums will only put the brace on its own line if 
the existing attachment rules indicate that it should.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99840

Files:
  clang/include/clang/Format/Format.h
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestCSharp.cpp


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -343,8 +343,7 @@
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-   "{\n"
+  verifyFormat("public enum var {\n"
"none,\n"
"@string,\n"
"bool,\n"
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1344,6 +1344,14 @@
   Style.AllowShortEnumsOnASingleLine = true;
   verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
   Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum {\n"
+   "  A,\n"
+   "  B,\n"
+   "  C\n"
+   "} ShortEnum1, ShortEnum2;",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
   verifyFormat("enum\n"
"{\n"
"  A,\n"
@@ -19454,8 +19462,7 @@
Style);
   // Enumerations are not records and should be unaffected.
   Style.AllowShortEnumsOnASingleLine = false;
-  verifyFormat("enum class E\n"
-   "{\n"
+  verifyFormat("enum class E {\n"
"  A,\n"
"  B\n"
"};\n",
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2455,6 +2455,8 @@
   if (FormatTok->Tok.is(tok::kw_enum))
 nextToken();
 
+  const FormatToken &InitialToken = *FormatTok;
+
   // In TypeScript, "enum" can also be used as property name, e.g. in interface
   // declarations. An "enum" keyword followed by a colon would be a syntax
   // error and thus assume it is just an identifier.
@@ -2501,7 +2503,8 @@
 return true;
   }
 
-  if (!Style.AllowShortEnumsOnASingleLine)
+  if (!Style.AllowShortEnumsOnASingleLine &&
+  ShouldBreakBeforeBrace(Style, InitialToken))
 addUnwrappedLine();
   // Parse enum body.
   nextToken();
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -515,8 +515,7 @@
   ///   enum { A, B } myEnum;
   ///
   ///   false:
-  ///   enum
-  ///   {
+  ///   enum {
   /// A,
   /// B
   ///   } myEnum;


Index: clang/unittests/Format/FormatTestCSharp.cpp
===
--- clang/unittests/Format/FormatTestCSharp.cpp
+++ clang/unittests/Format/FormatTestCSharp.cpp
@@ -343,8 +343,7 @@
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-   "{\n"
+  verifyFormat("public enum var {\n"
"none,\n"
"@string,\n"
"bool,\n"
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1344,6 +1344,14 @@
   Style.AllowShortEnumsOnASingleLine = true;
   verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style);
   Style.AllowShortEnumsOnASingleLine = false;
+  verifyFormat("enum {\n"
+   "  A,\n"
+   "  B,\n"
+   "  C\n"
+   "} ShortEnum1, ShortEnum2;",
+   Style);
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = true;
   verifyFormat("enum\n"
"{\n"
"  A,\n"
@@ -19454,8 +19462,7 @@
Style);
   // Enumerations are not records and should be unaffected.
   Style.AllowShortEnumsOnASingleLine = false;
-  verifyFormat("enum class E\n"
-   "{\n"
+  verifyFormat("enum class E {\n"
"  A,\n"
"  B\n"
"};\n",
Index: clang/lib/Fo