[clang-format] respect AfterEnum for enums

[m1cha]

Summary

There is some similar looking code in TokenAnnotator.cpp but given that I've
never worked on clang-format before I don't know what the purpose of that code
is and how it's related to UnwrappedLineParser.cpp.

Either way, it fixes clang-format with BraceWrapping.AfterEnum=true and
AllowShortEnumsOnASingleLine=false to behave like the documentation says.

Before this patch:

enum
{
  A,
  B
} myEnum;

After this patch:

enum {
  A,
  B
} myEnum;

According to the unittests which I had to modify this would change the LLVM
style. Please evaluate if you want to change the defaults or if you consider
the current style a bug.

[m1cha]

I just noticed that phabricator strips the commit message so here it is:

There is some similar looking code in TokenAnnotator.cpp but given that I've
never worked on clang-format before I don't know what the purpose of that code
is and how it's related to UnwrappedLineParser.cpp.

Either way, it fixes clang-format with BraceWrapping.AfterEnum=true and
AllowShortEnumsOnASingleLine=false to behave like the documentation says.

Before this patch:

enum
{
  A,
  B
} myEnum;

After this patch:

enum {
  A,
  B
} myEnum;

According to the unittests which I had to modify this would change the LLVM
style. Please evaluate if you want to change the defaults or if you consider
the current style a bug.

[MyDeveloperDay]

This feels like there is some overlap with D93938: [clang-format] Fixed AfterEnum handling

[HazardyKnusperkeks]

In D106349#2893672, @MyDeveloperDay wrote:

This feels like there is some overlap with D93938: [clang-format] Fixed AfterEnum handling

Yeah that may be, but it is stale for over a month, with I think open questions. So if this here fixes at least one bug with no obvious new ones, I think we should go for it.

Regarding the patch: Please upload it with the context available. https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[m1cha]

• rebased to main
• fixed the test IndentAccessModifiers

[HazardyKnusperkeks]

Looks good, could you also add a note in the relasenotes.rst?

[m1cha]

In D106349#2897400, @HazardyKnusperkeks wrote:

Looks good, could you also add a note in the relasenotes.rst?

Sure

Can I do something about the failing unittests? I don't know how they are related to the changes I made.

[HazardyKnusperkeks]

In D106349#2900770, @m1cha wrote:

In D106349#2897400, @HazardyKnusperkeks wrote:

Looks good, could you also add a note in the relasenotes.rst?

Sure

Can I do something about the failing unittests? I don't know how they are related to the changes I made.

They (most likely) aren't. That's something which set me off at the beginning too. If the Format Tests pass on your machine most probably everything is fine for you and for clang-format.
When you update the patch with release notes you can rebase on current main, maybe it is already fixed, maybe not.

[m1cha]

I've added a note to ReleaseNotes.rst

[HazardyKnusperkeks]

No one objects, you can push it. Or if you don't have commit access (and don't want to, or don't want to wait for it) we can push it, then please state name and email for the commit.

[m1cha]

I don't have commit access. How does that even work? The documentation is very scarce about this but for security reasons I'd expect there to be a very limited set of people with commit access

Here's my author info:
Michael Zimmermann <[e-mail]>

[lunasorcery]

Note also there's significant overlap with the now-committed D99840, though that's missing the change inside ShouldBreakBeforeBrace().
In retrospect I'm a little confused as to why D99840 appears to work without it.

[HazardyKnusperkeks]

In D106349#2914448, @m1cha wrote:

I don't have commit access. How does that even work? The documentation is very scarce about this but for security reasons I'd expect there to be a very limited set of people with commit access

Here's my author info:
Michael Zimmermann <[e-mail]>

https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access I don't think it is limited, I'm also just some guy which applied for it some 7 months ago. Since the next release is far away, you can apply for the access and commit it yourself. I did only wait one day. ;)

In D106349#2914500, @lunasorcery wrote:

Note also there's significant overlap with the now-committed D99840, though that's missing the change inside ShouldBreakBeforeBrace().
In retrospect I'm a little confused as to why D99840 appears to work without it.

I thought it looked familiar, but did not find what it was. Does this change needs to be rebased before it can be applied?

[m1cha]

Well it's pretty much exactly the same except for the change in ShouldBreakBeforeBrace.
Somebody with more knowledge about clang-format than me should try to figure out why it's not needed.

If it's actually not needed than this change does not need to be merged.

[lunasorcery]

Copying over the comment I left on D99840 explaining it - my existing change only works because it gets bailed out by further code in TokenAnnotator.cpp.
I think your change still is valuable since it's more correct.

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 [D99840] _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.

[curdeius]

LGTM. If nobody objects, I'll land this today.

[MyDeveloperDay]

no objection from me, we need to clean up the non landed accepted reviews or it wastes our time in the first place.

clang/lib/Format/UnwrappedLineParser.cpp

static bool ShouldBreakBeforeBrace(const FormatStyle &Style,
   if (InitialToken.isOneOf(tok::kw_namespace, TT_NamespaceMacro))
     return Style.BraceWrapping.AfterNamespace;
   if (InitialToken.is(tok::kw_class))
     return Style.BraceWrapping.AfterClass;
   if (InitialToken.is(tok::kw_union))
     return Style.BraceWrapping.AfterUnion;
   if (InitialToken.is(tok::kw_struct))
     return Style.BraceWrapping.AfterStruct;
+  if (InitialToken.is(tok::kw_enum))
+    return Style.BraceWrapping.AfterEnum;
   return false;
 }
 
 void UnwrappedLineParser::parseChildBlock() {
   FormatTok->setBlockKind(BK_Block);
   nextToken();
   {
     bool SkipIndent = (Style.Language == FormatStyle::LK_JavaScript &&
if (FormatTok->Previous && FormatTok->Previous->is(tok::greater)) {
     }
   }
   nextToken();
 
   parseRequiresExpression(OriginalLevel);
 }
 
 bool UnwrappedLineParser::parseEnum() {
+  const FormatToken &InitialToken = *FormatTok;
+
   // Won't be 'enum' for NS_ENUMs.
   if (FormatTok->Tok.is(tok::kw_enum))
     nextToken();
 
   // 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.
   if (Style.Language == FormatStyle::LK_JavaScript &&
if (Style.Language == FormatStyle::LK_Java) {
     parseJavaEnumBody();
     return true;
   }
   if (Style.Language == FormatStyle::LK_Proto) {
     parseBlock(/*MustBeDeclaration=*/true);
     return true;
   }
 
-  if (!Style.AllowShortEnumsOnASingleLine)
+  if (!Style.AllowShortEnumsOnASingleLine &&
+      ShouldBreakBeforeBrace(Style, InitialToken))
     addUnwrappedLine();
   // Parse enum body.
   nextToken();
   if (!Style.AllowShortEnumsOnASingleLine) {
     addUnwrappedLine();
     Line->Level += 1;
   }
   bool HasError = !parseBracedList(/*ContinueOnSemicolons=*/true,

clang/unittests/Format/FormatTest.cpp

verifyFormat("switch (x) {\n"
                "}");
 }
 
 TEST_F(FormatTest, ShortEnums) {
   FormatStyle Style = getLLVMStyle();
   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"
                "  B,\n"
                "  C\n"
                "} ShortEnum1, ShortEnum2;",
                Style);
 }
verifyFormat("struct S {\n"
                "    };\n"
                "\n"
                "  public:\n"
                "    int i;\n"
                "};\n",
                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",
                Style);
   // Test with a different indentation width;
   // also proves that the result is Style.AccessModifierOffset agnostic.
   Style.IndentWidth = 3;
   verifyFormat("class C {\n"