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

[lunasorcery]

Summary

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.

[HazardyKnusperkeks]

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.

[MyDeveloperDay]

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.

[curdeius]

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 <email@address>" for commit attribution, otherwise we'll use the name/email associated with you Phabricator account.

[HazardyKnusperkeks]

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 <email@address>" for commit attribution, otherwise we'll use the name/email associated with you Phabricator account.

What he says.

[lunasorcery]

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

[lunasorcery]

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.

[MyDeveloperDay]

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

clang/include/clang/Format/Format.h

547

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.

clang/docs/ReleaseNotes.rst

 AST Matchers
 ------------
 
 - ...
 
 clang-format
 ------------
 
-- ...
+- Option ``AllowShortEnumsOnASingleLine: false`` has been improved, it now
+  correctly places the opening brace according to ``BraceWrapping.AfterEnum``.
 
 libclang
 --------
 
 - ...
 
 Static Analyzer
 ---------------

clang/include/clang/Format/Format.h

struct FormatStyle {
   bool AllowAllParametersOfDeclarationOnNextLine;
 
   /// Allow short enums on a single line.
   /// \code
   ///   true:
   ///   enum { A, B } myEnum;
   ///
   ///   false:
-  ///   enum
-  ///   {
+  ///   enum {
   ///     A,
   ///     B
   ///   } myEnum;
   /// \endcode
   bool AllowShortEnumsOnASingleLine;
 
   /// Different styles for merging short blocks containing at most one
   /// statement.

clang/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseRequires() {
   parseRequiresExpression(OriginalLevel);
 }
 
 bool UnwrappedLineParser::parseEnum() {
   // Won't be 'enum' for NS_ENUMs.
   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.
   if (Style.Language == FormatStyle::LK_JavaScript &&
       FormatTok->isOneOf(tok::colon, tok::question))
     return false;
 
   // In protobuf, "enum" can be used as a field name.
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"

clang/unittests/Format/FormatTestCSharp.cpp

 }
 
 TEST_F(FormatTestCSharp, CSharpRegions) {
   verifyFormat("#region aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa aaaaaaaaaaaaaaaaa "
                "aaaaaaaaaaaaaaa long region");
 }
 
 TEST_F(FormatTestCSharp, CSharpKeyWordEscaping) {
-  verifyFormat("public enum var\n"
-               "{\n"
+  verifyFormat("public enum var {\n"
                "    none,\n"
                "    @string,\n"
                "    bool,\n"
                "    @enum\n"
                "}");
 }
 
 TEST_F(FormatTestCSharp, CSharpNullCoalescing) {