[clang-format] [PR19056] Add support for indenting class members and methods one level under the modifiers

[MyDeveloperDay]

Summary

Code::Blocks and Qt Creator code uses a style guide which recommends indenting the next line after the access modifier e.g.

class Foo
{
          int abc;

      public:
          int def;
          void foo();

      private:
          int ghi;
          void foo();
}

The following patch with the addition of a new option

AccessModifierIndentation: true

This PR (https://bugs.llvm.org/show_bug.cgi?id=19056) was raised a long time ago (2014), and recently was pinged for an update

The following patch aim to address this issue, potentially allowing clang-format to be used (rather than astyle)

[klimek]

clang/include/clang/Format/Format.h

50

I think we need to explain this a bit more:
What this does is:
Indent classes with access modifiers at 2x indent compared to classes without access modifiers, while keeping the access modifiers at a normal indent.

clang/lib/Format/UnwrappedLineParser.cpp

2009

What if the class starts at level 1? (for example, inside a function or due to namespace indentation)

namespace A {

class B {
  public:
    ..
};

}

2022

Similarly, I think we need to remember whether we unindented, as otherwise I think we can run into cases where this is true, but the previous was false (class declared at level > 1).

2230

We should structure this like other parse loops in this file, using switch and eof (instead of !Next). See parseParens() for a good example.

[reuk]

clang/lib/Format/UnwrappedLineParser.cpp

2009

Probably worth adding a test or two that format nested classes, classes inside namespaces, classes defined inside functions etc.

2253

Yeah, this would look way nicer using a settings struct.

clang/lib/Format/UnwrappedLineParser.h

89

Adjacent bool parameters is a bit nasty. Maybe pass a struct of settings instead? https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#i4-make-interfaces-precisely-and-strongly-typed

[3nids]

Hi, may I ask why this is abandoned?
We are eagerly waiting on this to move to clang-format.

[MyDeveloperDay]

It was more about not having time to pursue it at this time.. I didn't feel I should hog the PR if someone else wanted to take a look.

Even though its "Abandoned" it can be brought back at anytime, I don't like to sit on reviews for too long as it doesn't show the correct intent. I'm happy for others to take it over, but I don't have a personal need for this.

[3nids]

Hi there, is there anything we can do to help this to go through?

clang/docs/ClangFormatStyleOptions.rst

* ``WebKit``
     A style complying with `WebKit's style guide
     <https://www.webkit.org/coding/coding-style.html>`_
   * ``Microsoft``
     A style complying with `Microsoft's style guide
     <https://docs.microsoft.com/en-us/visualstudio/ide/editorconfig-code-style-settings-reference?view=vs-2017>`_
 
 .. START_FORMAT_STYLE_OPTIONS
 
+**AccessModifierIndentation** (``bool``)
+  Indents after access modifiers. i.e.
+
+  .. code-block:: c++
+
+     false:                                 true:
+     public:              vs.               public:
+     foo();                                     foo();
+
 **AccessModifierOffset** (``int``)
   The extra indent or outdent of access modifiers, e.g. ``public:``.
 
 **AlignAfterOpenBracket** (``BracketAlignmentStyle``)
   If ``true``, horizontally aligns arguments after an open bracket.
 
   This applies to round brackets (parentheses), angle brackets and square
   brackets.

clang/include/clang/Format/Format.h

public:
   std::string message(int EV) const override;
 };
 const std::error_category &getParseCategory();
 std::error_code make_error_code(ParseError e);
 
 /// The ``FormatStyle`` is used to configure the formatting to follow
 /// specific guidelines.
 struct FormatStyle {
+  /// Indents after access modifiers. i.e.
+  /// \code
+  ///    false:                                 true:
+  ///    public:              vs.               public:
+  ///    foo();                                     foo();
+  /// \endcode
+  bool AccessModifierIndentation;
+
   /// The extra indent or outdent of access modifiers, e.g. ``public:``.
   int AccessModifierOffset;
 
   /// Different styles for aligning after open brackets.
   enum BracketAlignmentStyle {
     /// Align parameters on the open bracket, e.g.:
     /// \code
     ///   someLongFunction(argument1,
struct FormatStyle {
   ///    aaaa =                         vs.     aaaa = "bbbb"
   ///        "bbbb"                                    "cccc";
   ///        "cccc";
   /// \endcode
   bool AlwaysBreakBeforeMultilineStrings;
 
   /// Different ways to break after the template declaration.
   enum BreakTemplateDeclarationsStyle {
-      /// Do not force break before declaration.
-      /// ``PenaltyBreakTemplateDeclaration`` is taken into account.
-      /// \code
-      ///    template <typename T> T foo() {
-      ///    }
-      ///    template <typename T> T foo(int aaaaaaaaaaaaaaaaaaaaa,
-      ///                                int bbbbbbbbbbbbbbbbbbbbb) {
-      ///    }
-      /// \endcode
-      BTDS_No,
-      /// Force break after template declaration only when the following
-      /// declaration spans multiple lines.
-      /// \code
-      ///    template <typename T> T foo() {
-      ///    }
-      ///    template <typename T>
-      ///    T foo(int aaaaaaaaaaaaaaaaaaaaa,
-      ///          int bbbbbbbbbbbbbbbbbbbbb) {
-      ///    }
-      /// \endcode
-      BTDS_MultiLine,
-      /// Always break after template declaration.
-      /// \code
-      ///    template <typename T>
-      ///    T foo() {
-      ///    }
-      ///    template <typename T>
-      ///    T foo(int aaaaaaaaaaaaaaaaaaaaa,
-      ///          int bbbbbbbbbbbbbbbbbbbbb) {
-      ///    }
-      /// \endcode
-      BTDS_Yes
   };
 
   /// The template declaration breaking style to use.
   BreakTemplateDeclarationsStyle AlwaysBreakTemplateDeclarations;
 
   /// If ``false``, a function call's arguments will either be all on the
   /// same line or will have one line each.
   /// \code
enum UseTabStyle {
     /// one tab stop to the next one.
     UT_Always
   };
 
   /// The way to use tab characters in the resulting file.
   UseTabStyle UseTab;
 
   bool operator==(const FormatStyle &R) const {
-    return AccessModifierOffset == R.AccessModifierOffset &&
+    return AccessModifierIndentation == R.AccessModifierIndentation &&
+           AccessModifierOffset == R.AccessModifierOffset &&
            AlignAfterOpenBracket == R.AlignAfterOpenBracket &&
            AlignConsecutiveAssignments == R.AlignConsecutiveAssignments &&
            AlignConsecutiveDeclarations == R.AlignConsecutiveDeclarations &&
            AlignEscapedNewlines == R.AlignEscapedNewlines &&
            AlignOperands == R.AlignOperands &&
            AlignTrailingComments == R.AlignTrailingComments &&
            AllowAllArgumentsOnNextLine == R.AllowAllArgumentsOnNextLine &&
            AllowAllConstructorInitializersOnNextLine ==

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("JavaScriptQuotes", Style.JavaScriptQuotes);
     IO.mapOptional("JavaScriptWrapImports", Style.JavaScriptWrapImports);
     IO.mapOptional("KeepEmptyLinesAtTheStartOfBlocks",
                    Style.KeepEmptyLinesAtTheStartOfBlocks);
     IO.mapOptional("MacroBlockBegin", Style.MacroBlockBegin);
     IO.mapOptional("MacroBlockEnd", Style.MacroBlockEnd);
     IO.mapOptional("MaxEmptyLinesToKeep", Style.MaxEmptyLinesToKeep);
     IO.mapOptional("NamespaceIndentation", Style.NamespaceIndentation);
+    IO.mapOptional("AccessModifierIndentation",
+                   Style.AccessModifierIndentation);
     IO.mapOptional("ObjCBinPackProtocolList", Style.ObjCBinPackProtocolList);
     IO.mapOptional("ObjCBlockIndentWidth", Style.ObjCBlockIndentWidth);
     IO.mapOptional("ObjCSpaceAfterProperty", Style.ObjCSpaceAfterProperty);
     IO.mapOptional("ObjCSpaceBeforeProtocolList",
                    Style.ObjCSpaceBeforeProtocolList);
     IO.mapOptional("PenaltyBreakAssignment", Style.PenaltyBreakAssignment);
     IO.mapOptional("PenaltyBreakBeforeFirstCallParameter",
                    Style.PenaltyBreakBeforeFirstCallParameter);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;
   LLVMStyle.JavaScriptQuotes = FormatStyle::JSQS_Leave;
   LLVMStyle.JavaScriptWrapImports = true;
   LLVMStyle.TabWidth = 8;
   LLVMStyle.MaxEmptyLinesToKeep = 1;
   LLVMStyle.KeepEmptyLinesAtTheStartOfBlocks = true;
   LLVMStyle.NamespaceIndentation = FormatStyle::NI_None;
+  LLVMStyle.AccessModifierIndentation = false;
   LLVMStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Auto;
   LLVMStyle.ObjCBlockIndentWidth = 2;
   LLVMStyle.ObjCSpaceAfterProperty = false;
   LLVMStyle.ObjCSpaceBeforeProtocolList = true;
   LLVMStyle.PointerAlignment = FormatStyle::PAS_Right;
   LLVMStyle.SpacesBeforeTrailingComments = 1;
   LLVMStyle.Standard = FormatStyle::LS_Cpp11;
   LLVMStyle.UseTab = FormatStyle::UT_Never;

clang/lib/Format/UnwrappedLineParser.h

public:
 
   void parse();
 
 private:
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
   void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
-                  bool MunchSemi = true);
+                  bool MunchSemi = true, bool IndentedAccessModifiers = false);
   void parseChildBlock();
   void parsePPDirective();
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();

clang/lib/Format/UnwrappedLineParser.cpp

size_t UnwrappedLineParser::computePPHash() const {
   for (const auto &i : PPStack) {
     hash_combine(h, size_t(i.Kind));
     hash_combine(h, i.Line);
   }
   return h;
 }
 
 void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
-                                     bool MunchSemi) {
+                                     bool MunchSemi,
+                                     bool IndentedAccessModifiers) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
          "'{' or macro block token expected");
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
   FormatTok->BlockKind = BK_Block;
 
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
size_t OpeningLineIndex =
       CurrentLines->empty()
           ? (UnwrappedLine::kInvalidIndex)
           : (CurrentLines->size() - 1 - NbPreprocessorDirectives);
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
                                           MustBeDeclaration);
   if (AddLevel)
     ++Line->Level;
+
+  if (IndentedAccessModifiers)
+    ++Line->Level;
+
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
     return;
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
     Line->Level = InitialLevel;
if (FormatTok->Tok.is(tok::l_brace)) {
     addUnwrappedLine();
     ++Line->Level;
     parseStructuralElement();
     --Line->Level;
   }
 }
 
 void UnwrappedLineParser::parseAccessSpecifier() {
+  // Indentation after an access modifier unindent one level
+  // if already in indented scope
+  // uneven/odd level nested access modifiers handles inner
+  // classes case
+  if (Style.AccessModifierIndentation && Line->Level % 2 == 0)
+    --Line->Level;
+
   nextToken();
   // Understand Qt's slots.
   if (FormatTok->isOneOf(Keywords.kw_slots, Keywords.kw_qslots))
     nextToken();
   // Otherwise, we don't know what it is, and we'd better keep the next token.
   if (FormatTok->Tok.is(tok::colon))
     nextToken();
   addUnwrappedLine();
+
+  // After we wrap for Access modifier then indent a level if desired
+  if (Style.AccessModifierIndentation && Line->Level >= 1)
+    ++Line->Level;
 }
 
 bool UnwrappedLineParser::parseEnum() {
   // 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
void UnwrappedLineParser::parseRecord(bool ParseAsExpr) {
   }
   if (FormatTok->Tok.is(tok::l_brace)) {
     if (ParseAsExpr) {
       parseChildBlock();
     } else {
       if (ShouldBreakBeforeBrace(Style, InitialToken))
         addUnwrappedLine();
 
+      bool ContainsAccessModifier = false;
+
+      // If the block is a class or a struct, scan to the matching `}` to
+      // determine if it has any access modifiers, if so we must double the
+      // indent level of the class ahead of the first modifier, to ensure
+      // implicitly private members are aligned with indented members after
+      // first modifier
+      if (Style.AccessModifierIndentation &&
+          InitialToken.isOneOf(tok::kw_class, tok::kw_struct)) {
+        unsigned StoredPosition = Tokens->getPosition();
+        FormatToken *Next;
+        int BraceLevel = 1;
+        do {
+          Next = Tokens->getNextToken();
+          if (!Next)
+            break;
+          // Keep track of the token range of class or struct.
+          if (Next->is(tok::l_brace))
+            ++BraceLevel;
+          if (Next->is(tok::r_brace)) {
+            --BraceLevel;
+            if (BraceLevel == 0)
+              break;
+          }
+          // If any access specifiers or signals/slots (Qt) we will need
+          // to doubly indent
+          if (Next->isOneOf(tok::kw_public, tok::kw_private, tok::kw_protected,
+                            Keywords.kw_signals, Keywords.kw_qsignals,
+                            Keywords.kw_slots, Keywords.kw_qslots)) {
+            ContainsAccessModifier = true;
+            break;
+          }
+        } while (Next);
+        FormatTok = Tokens->setPosition(StoredPosition);
+      }
+
       parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true,
-                 /*MunchSemi=*/false);
+                 /*MunchSemi=*/false,
+                 /*IndentedAccessModifiers=*/ContainsAccessModifier);
     }
   }
   // There is no addUnwrappedLine() here so that we fall through to parsing a
   // structural element afterwards. Thus, in "class A {} n, m;",
   // "} n, m;" will end up in one unwrapped line.
 }
 
 void UnwrappedLineParser::parseObjCMethod() {

clang/unittests/Format/FormatTest.cpp

EXPECT_EQ("namespace out { namespace mid { namespace in {\n"
                    "namespace in {\n"
                    "int i;\n"
                    "} // namespace in\n"
                    "} // namespace mid\n"
                    "} // namespace out",
                    Style));
 }
 
+TEST_F(FormatTest, FormatAccessModifierIndentation) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AccessModifierIndentation = false;
+  EXPECT_EQ("class Foo1 {\n"
+            "  int i;\n"
+            "\n"
+            "public:\n"
+            "  int j;\n"
+            "  int k;\n"
+            "  Foo1() {}\n"
+            "\n"
+            "private:\n"
+            "  Bar() {}\n"
+            "  int l;\n"
+            "  int m;\n"
+            "\n"
+            "protected:\n"
+            "  Ping() {}\n"
+            "  int n;\n"
+            "  int o;\n"
+            "};\n",
+            format("class Foo1 {\n"
+                   "int i;\n"
+                   "public:\n"
+                   "int j;\n"
+                   "int k;\n"
+                   "Foo1() {}\n"
+                   "private:\n"
+                   "Bar() {}\n"
+                   "int l;\n"
+                   "int m;\n"
+                   "protected:\n"
+                   "Ping() {}\n"
+                   "int n;\n"
+                   "int o;\n"
+                   "};\n",
+                   Style));
+
+  Style.AccessModifierIndentation = true;
+  EXPECT_EQ("class Foo2 {\n"
+            "    int i;\n"
+            "\n"
+            "public:\n"
+            "    int j;\n"
+            "    int k;\n"
+            "    Foo2() {}\n"
+            "\n"
+            "private:\n"
+            "    Bar() {}\n"
+            "    int l;\n"
+            "    int m;\n"
+            "\n"
+            "protected:\n"
+            "    Ping() {}\n"
+            "    int n;\n"
+            "    int o;\n"
+            "};\n",
+            format("class Foo2 {\n"
+                   "int i;\n"
+                   "public:\n"
+                   "int j;\n"
+                   "int k;\n"
+                   "Foo2() {}\n"
+                   "private:\n"
+                   "Bar() {}\n"
+                   "int l;\n"
+                   "int m;\n"
+                   "protected:\n"
+                   "Ping() {}\n"
+                   "int n;\n"
+                   "int o;\n"
+                   "};\n",
+                   Style));
+
+  Style.BreakBeforeBraces = FormatStyle::BS_Allman;
+  Style.AccessModifierIndentation = false;
+  EXPECT_EQ("class Foo3\n"
+            "{\n"
+            "  int i;\n"
+            "\n"
+            "public:\n"
+            "  int j;\n"
+            "  int k;\n"
+            "  Foo3() {}\n"
+            "\n"
+            "private:\n"
+            "  Bar() {}\n"
+            "  int l;\n"
+            "  int m;\n"
+            "\n"
+            "protected:\n"
+            "  Ping() {}\n"
+            "  int n;\n"
+            "  int o;\n"
+            "};\n",
+            format("class Foo3 {\n"
+                   "int i;\n"
+                   "public:\n"
+                   "int j;\n"
+                   "int k;\n"
+                   "Foo3() {}\n"
+                   "private:\n"
+                   "Bar() {}\n"
+                   "int l;\n"
+                   "int m;\n"
+                   "protected:\n"
+                   "Ping() {}\n"
+                   "int n;\n"
+                   "int o;\n"
+                   "};\n",
+                   Style));
+
+  Style.AccessModifierIndentation = true;
+  EXPECT_EQ("class Foo4\n"
+            "{\n"
+            "    int i;\n"
+            "\n"
+            "public:\n"
+            "    int j;\n"
+            "    int k;\n"
+            "    Foo4() {}\n"
+            "\n"
+            "private:\n"
+            "    Bar() {}\n"
+            "    int l;\n"
+            "    int m;\n"
+            "\n"
+            "protected:\n"
+            "    Ping() {}\n"
+            "    int n;\n"
+            "    int o;\n"
+            "};\n",
+            format("class Foo4 {\n"
+                   "int i;\n"
+                   "public:\n"
+                   "int j;\n"
+                   "int k;\n"
+                   "Foo4() {}\n"
+                   "private:\n"
+                   "Bar() {}\n"
+                   "int l;\n"
+                   "int m;\n"
+                   "protected:\n"
+                   "Ping() {}\n"
+                   "int n;\n"
+                   "int o;\n"
+                   "};\n",
+                   Style));
+
+  EXPECT_EQ("class Foo5\n"
+            "{\n"
+            "    int i;\n"
+            "\n"
+            "public:\n"
+            "    int j;\n"
+            "    int k;\n"
+            "    Foo5() {}\n"
+            "    class Inner\n"
+            "    {\n"
+            "    private:\n"
+            "        Bar() {}\n"
+            "        int l;\n"
+            "        int m;\n"
+            "\n"
+            "    protected:\n"
+            "        Ping() {}\n"
+            "        int n;\n"
+            "        int o;\n"
+            "    };\n"
+            "};\n",
+            format("class Foo5 {\n"
+                   "int i;\n"
+                   "public:\n"
+                   "int j;\n"
+                   "int k;\n"
+                   "Foo5() {}\n"
+                   "class Inner {\n"
+                   "private:\n"
+                   "Bar() {}\n"
+                   "int l;\n"
+                   "int m;\n"
+                   "protected:\n"
+                   "Ping() {}\n"
+                   "int n;\n"
+                   "int o;\n"
+                   "};\n"
+                   "};\n",
+                   Style));
+
+  EXPECT_EQ("struct Foo6\n"
+            "{\n"
+            "  int i;\n"
+            "  int j;\n"
+            "  int k;\n"
+            "};\n",
+            format("struct Foo6 {\n"
+                   "int i;\n"
+                   "int j;\n"
+                   "int k;\n"
+                   "};\n",
+                   Style));
+}
+
 TEST_F(FormatTest, FormatsExternC) {
   verifyFormat("extern \"C\" {\nint a;");
   verifyFormat("extern \"C\" {}");
   verifyFormat("extern \"C\" {\n"
                "int foo();\n"
                "}");
   verifyFormat("extern \"C\" int foo() {}");
   verifyFormat("extern \"C\" int foo();");
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(SpacesInCStyleCastParentheses);
   CHECK_PARSE_BOOL(SpaceAfterCStyleCast);
   CHECK_PARSE_BOOL(SpaceAfterTemplateKeyword);
   CHECK_PARSE_BOOL(SpaceBeforeAssignmentOperators);
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
+  CHECK_PARSE_BOOL(AccessModifierIndentation);
 
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterClass);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterControlStatement);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterEnum);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterFunction);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterNamespace);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterObjCDeclaration);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, AfterStruct);