[clang-format] [PR19056] Add support for access modifiers indentation

[Budovi]

Summary

Adds support for coding styles that make a separate indentation level for access modifiers, such as Code::Blocks or QtCreator.

The new option, IndentAccessModifiers, if enabled, forces the content inside classes, structs and unions (“records”) to be indented twice while removing a level for access modifiers. The value of AccessModifierOffset is disregarded in this case, aiming towards an ease of use.

The PR (https://bugs.llvm.org/show_bug.cgi?id=19056) had an implementation attempt by @MyDeveloperDay already (https://reviews.llvm.org/D60225) but I've decided to start from scratch. They differ in functionality, chosen approaches, and even the option name. The code tries to re-use the existing functionality to achieve this behavior, limiting possibility of breaking something else.

[HazardyKnusperkeks]

I would add a test where you have a member before the first access modifier.
Also this is not exactly what they wanted in the bug, as far as I can see members of structs or classes with no access modifier at all should only be indented once.

[Budovi]

In D94661#2497744, @HazardyKnusperkeks wrote:

I would add a test where you have a member before the first access modifier.

The very first unit test has no modifiers, the third one has a member before a modifier in the nested class C.

In D94661#2497744, @HazardyKnusperkeks wrote:

Also this is not exactly what they wanted in the bug, as far as I can see members of structs or classes with no access modifier at all should only be indented once.

I don't think there is a consensus about how this should actually work, see e. g. https://bugs.llvm.org/show_bug.cgi?id=19056#c11 .

I see qtcreator's style a bit more relevant than some astyle configuration of a specific project. I also find it personally ugly, can't use it myself and thus would have to make it configurable. In that case, this change could be viewed as a "possible stepping stone", even though it would probably require a major overhaul of the logic.

The previous patch, if I read it correctly, implements this detail in the same way as I did.

clang/unittests/Format/FormatTest.cpp

17882–17908

When you re-create this test using verifyFormat, you get a weird results, i.e. if you keep the newlines before access modifiers, the test fails because it thinks they should not be there, but if you remove them, test fails again, because the formatter includes them instead.

It's a good question whether it is a side-effect of test::messUp or a different bug, I'm not sure.

17914–17918

This unit test currently fails, I believe it is a non-related bug in the formatter. Nevertheless I wanted to add a check that enums (non-records) are preserved.

[curdeius]

That looks like a strange style option to me. Is there any bigger codebase formatting the code this way?

clang/docs/ClangFormatStyleOptions.rst

2039

The name IndentAccessModifiers is misleading for me. Access modifiers are public, protected, private.

2039

Hmm, actually that's the description that is misleading.

clang/unittests/Format/FormatTest.cpp

17868

Why are those non-related style options here?

[Budovi]

clang/docs/ClangFormatStyleOptions.rst

2039

Hm. Will go through the bug report again to see how people describe it. I'm open to suggestions, but I'll need to fix it anyway because I see a grammar mistake.

The current way I tried to go about it is that, without this option, modifiers don't have their own indentation level. Without the offset given by the AccessModifierOffset they would just be flush with the record members.

The introduced option creates a level designated for the access modifiers by shifting the members further to the right.

clang/unittests/Format/FormatTest.cpp

17868

AccessModifierOffset tests whether it has an influence, even thought now realizing that the default value (-2) is non-zero and is probably good enough as arbitrary 4. So I actually want a non-zero value for it, and this was not obvious at first.

AllowShortEnumsOnASingleLine is needed unless I want to make the enum more complex.

BreakBeforeBraces along with BraceWrapping deals with the chosen enum test below, which had surprising defaults for me in the LLVMStyle. I've added them as a preliminary way of "resolving" the issue with the unit test but it still fails. I agree that the unit test should not try to cover the other features and I will remove them and fix the test as soon as I get into it and confirm that the failing test is not a bug introduced with this change and open up a bug report for it.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

17874

So this is indented 4 because the Access modifier is 4? when the IndentWidth is 2?

but public below is indented 2.. so now I'm really confused?

17882–17908

As a drive by comment, its my experience when verifyFormat fails its often a sign that there is a bug which means clang-format cannot always format from arbitrary text, which really it should.

Ultimately this bug will get logged by someone who doesn't understand your change as well as you do. That fix will also subtly end up creating a different bug in another corner case you you haven't covered in your original tests

We spiral down from there...my 2c worth.

[Budovi]

clang/unittests/Format/FormatTest.cpp

17868

Confirmed that the bug is present without this patch.
Related bug report: https://bugs.llvm.org/show_bug.cgi?id=46927

17874

The indentation is 2 levels below the class since the single level is reserved for access modifiers.

Access modifier does not influence the behaviour at all. The value was chosen arbitrarily, see my previous comment. Will reflect this in an updated patch.

17882–17908

This issue was caused by an inconsistency with access modifiers and unrelated to this patch. After the test::messUp all the newlines are stripped from the code, and clang-format behaved differently in a case there was no newline before access modifier (inserted a single one) and all the other cases (it inserted an additional empty line before the modifier).

The issue was fixed along with a new feature introduced https://reviews.llvm.org/D93846 and thus does not need reporting. The new patch will revert back to using verifyFormat.

17914–17918

As mentioned above, this is the bug https://bugs.llvm.org/show_bug.cgi?id=46927.
The unrelated style options will be removed in the future patch and the "default LLVM behaviour" tested.

[Budovi]

Updated the tests after checking the issues were not caused by the patch and they are reported / fixed.

Rebased the changes to the newest version.

[HazardyKnusperkeks]

You need to supply a full diff (with context).
Please also add it to the release notes.

[Budovi]

In D94661#2537356, @HazardyKnusperkeks wrote:

You need to supply a full diff (with context).
Please also add it to the release notes.

Oh sorry, forgot about the context lines. Will try to fix it tomorrow, along with the release notes.

[Budovi]

Changes:

• Added release notes
• Changed the documentation (tried to make it more clear in response to the confusion about the added option)

Hopefully there is a full context in the patch this time.

[Budovi]

clang/docs/ClangFormatStyleOptions.rst

2039

Hopefully the description is better this time.

[curdeius]

We're getting close :).

clang/unittests/Format/FormatTest.cpp

17867

Please add an assertion that IndentWidth == 2 to make the test easier to understand.

17897

Add a comment that enums are not concerned.

17910

Add tests with a different IndentWidth.

[curdeius]

Don't forget to mark comments as Done when... they're done :).

clang/docs/ClangFormatStyleOptions.rst

2039

Much better indeed! Thanks!

[Budovi]

clang/unittests/Format/FormatTest.cpp

17867

The written tests rely on several other "defaults" from the LLVM style. If somebody changes those defaults these unit tests (along with many others) will fail. This will also be true if an assertion is present to test this condition. It would be possible to reassign those values but this goes against how the rest of the tests are implemented.

But I agree that I could improve the readability by adding brief comments that point out important settings and the purpose of the included tests.

[Budovi]

Changes:

• Added a unit test with a different IndentWidth
• Removed a unit test that contained a class nested in function; didn't add much IMHO
• Added brief comments about potentially non-obvious behaviour to some tests

[curdeius]

It's getting into a good shape. Please fix the nits.

clang/unittests/Format/FormatTest.cpp

17870

Nit: please put full stops at the end of comments. Here and elsewhere.

17908

Typo: agnostic.

[Budovi]

Changes:

• Fixed comments

Thanks @curdeius for pointing out the issues.

[curdeius]

LGTM. But let other folks a few days to chime in. Especially @MyDeveloperDay's input would be valuable here as he already attempted to implement this.
Do you have have commit rights or you need somebody to land this for you? If it's the latter, please provide "Firstname Name <mail@domain>" for the commit attribution or we will just use the one associated with your Phabricator user.

[Budovi]

In D94661#2553843, @curdeius wrote:

LGTM. But let other folks a few days to chime in. Especially @MyDeveloperDay's input would be valuable here as he already attempted to implement this.

Sure, no problem. Thanks for your input once again :), really appreciate it.

In D94661#2553843, @curdeius wrote:

Do you have have commit rights or you need somebody to land this for you? If it's the latter, please provide "Firstname Name <mail@domain>" for the commit attribution or we will just use the one associated with your Phabricator user.

I don't have commit rights (this is my first contribution… well an attempt anyway) but the Phabricator user settings, i.e. something along Jakub Budiský <[e-mail]>, should be fine.

[MyDeveloperDay]

the LGTM, please mark your comments as done when done.

clang/docs/ClangFormatStyleOptions.rst

**IncludeIsMainSourceRegex** (``std::string``)
   For example, if this option is configured to ``(Impl\.hpp)$``,
   then a file ``ClassImpl.hpp`` is considered "main" (in addition to
   ``Class.c``, ``Class.cc``, ``Class.cpp`` and so on) and "main
   include file" logic will be executed (with *IncludeIsMainRegex* setting
   also being respected in later phase). Without this option set,
   ``ClassImpl.hpp`` would not have the main include file put on top
   before any other include.
 
+**IndentAccessModifiers** (``bool``)
+  Makes an indentation level for record (``class``, ``struct``, ``union``)
+  access modifiers.
+
+  When ``false``, access modifiers are indented relative to the member
+  indentation, respecting the ``AccessModifierOffset``.
+  When ``true``, access modifiers get their own indentation level. Record
+  members are always indented 2 levels below the record, regardless of the
+  access modifier presence. Value of ``AccessModifierOffset`` is ignored.
+
+  .. code-block:: c++
+
+     false:                                 true:
+     class C {                      vs.     class C {
+       class D {                                class D {
+         void bar();                                void bar();
+       protected:                                 protected:
+         D();                                       D();
+       };                                       };
+     public:                                  public:
+       C();                                     C();
+     };                                     };
+     void foo() {                           void foo() {
+       return 1;                              return 1;
+     }                                      }
+
 **IndentCaseBlocks** (``bool``)
   Indent case label blocks one level from the case label.
 
   When ``false``, the block following the case label uses the same
   indentation level as for the case label, treating the case label the same
   as an if-statement.
   When ``true``, the block gets indented as a scope block.
 

clang/include/clang/Format/Format.h

   ///   WhitespaceSensitiveMacros: ['STRINGIZE', 'PP_STRINGIZE']
   /// \endcode
   ///
   /// For example: BOOST_PP_STRINGIZE
   std::vector<std::string> WhitespaceSensitiveMacros;
 
   tooling::IncludeStyle IncludeStyle;
 
+  /// Makes an indentation level for record (``class``, ``struct``, ``union``)
+  /// access modifiers.
+  ///
+  /// When ``false``, access modifiers are indented relative to the member
+  /// indentation, respecting the ``AccessModifierOffset``.
+  /// When ``true``, access modifiers get their own indentation level. Record
+  /// members are always indented 2 levels below the record, regardless of the
+  /// access modifier presence. Value of ``AccessModifierOffset`` is ignored.
+  /// \code
+  ///    false:                                 true:
+  ///    class C {                      vs.     class C {
+  ///      class D {                                class D {
+  ///        void bar();                                void bar();
+  ///      protected:                                 protected:
+  ///        D();                                       D();
+  ///      };                                       };
+  ///    public:                                  public:
+  ///      C();                                     C();
+  ///    };                                     };
+  ///    void foo() {                           void foo() {
+  ///      return 1;                              return 1;
+  ///    }                                      }
+  /// \endcode
+  bool IndentAccessModifiers;
+
   /// Indent case labels one level from the switch statement.
   ///
   /// When ``false``, use the same indentation level as for the switch
   /// statement. Switch statement body is always indented one level more than
   /// case labels (except the first block following the case label, which
   /// itself indents the code - unless IndentCaseBlocks is enabled).
   /// \code
   ///    false:                                 true:
return AccessModifierOffset == R.AccessModifierOffset &&
            FixNamespaceComments == R.FixNamespaceComments &&
            ForEachMacros == R.ForEachMacros &&
            IncludeStyle.IncludeBlocks == R.IncludeStyle.IncludeBlocks &&
            IncludeStyle.IncludeCategories == R.IncludeStyle.IncludeCategories &&
            IncludeStyle.IncludeIsMainRegex ==
                R.IncludeStyle.IncludeIsMainRegex &&
            IncludeStyle.IncludeIsMainSourceRegex ==
                R.IncludeStyle.IncludeIsMainSourceRegex &&
+           IndentAccessModifiers == R.IndentAccessModifiers &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentCaseBlocks == R.IndentCaseBlocks &&
            IndentGotoLabels == R.IndentGotoLabels &&
            IndentPragmas == R.IndentPragmas &&
            IndentPPDirectives == R.IndentPPDirectives &&
            IndentExternBlock == R.IndentExternBlock &&
            IndentRequires == R.IndentRequires && IndentWidth == R.IndentWidth &&
            Language == R.Language &&

clang/lib/Format/Format.cpp

IO.mapOptional("ExperimentalAutoDetectBinPacking",
                    Style.ExperimentalAutoDetectBinPacking);
     IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
     IO.mapOptional("ForEachMacros", Style.ForEachMacros);
     IO.mapOptional("IncludeBlocks", Style.IncludeStyle.IncludeBlocks);
     IO.mapOptional("IncludeCategories", Style.IncludeStyle.IncludeCategories);
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
     IO.mapOptional("IncludeIsMainSourceRegex",
                    Style.IncludeStyle.IncludeIsMainSourceRegex);
+    IO.mapOptional("IndentAccessModifiers", Style.IndentAccessModifiers);
     IO.mapOptional("IndentCaseLabels", Style.IndentCaseLabels);
     IO.mapOptional("IndentCaseBlocks", Style.IndentCaseBlocks);
     IO.mapOptional("IndentGotoLabels", Style.IndentGotoLabels);
     IO.mapOptional("IndentPragmas", Style.IndentPragmas);
     IO.mapOptional("IndentPPDirectives", Style.IndentPPDirectives);
     IO.mapOptional("IndentExternBlock", Style.IndentExternBlock);
     IO.mapOptional("IndentRequires", Style.IndentRequires);
     IO.mapOptional("IndentWidth", Style.IndentWidth);
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.ForEachMacros.push_back("Q_FOREACH");
   LLVMStyle.ForEachMacros.push_back("BOOST_FOREACH");
   LLVMStyle.IncludeStyle.IncludeCategories = {
       {"^\"(llvm|llvm-c|clang|clang-c)/", 2, 0, false},
       {"^(<|\"(gtest|gmock|isl|json)/)", 3, 0, false},
       {".*", 1, 0, false}};
   LLVMStyle.IncludeStyle.IncludeIsMainRegex = "(Test)?$";
   LLVMStyle.IncludeStyle.IncludeBlocks = tooling::IncludeStyle::IBS_Preserve;
+  LLVMStyle.IndentAccessModifiers = false;
   LLVMStyle.IndentCaseLabels = false;
   LLVMStyle.IndentCaseBlocks = false;
   LLVMStyle.IndentGotoLabels = true;
   LLVMStyle.IndentPragmas = false;
   LLVMStyle.IndentPPDirectives = FormatStyle::PPDIS_None;
   LLVMStyle.IndentRequires = false;
   LLVMStyle.IndentWrappedFunctionNames = false;
   LLVMStyle.IndentWidth = 2;

clang/lib/Format/UnwrappedLineFormatter.cpp

private:
   /// characters to the left from their level.
   int getIndentOffset(const FormatToken &RootToken) {
     if (Style.Language == FormatStyle::LK_Java ||
         Style.Language == FormatStyle::LK_JavaScript || Style.isCSharp())
       return 0;
     if (RootToken.isAccessSpecifier(false) ||
         RootToken.isObjCAccessSpecifier() ||
         (RootToken.isOneOf(Keywords.kw_signals, Keywords.kw_qsignals) &&
-         RootToken.Next && RootToken.Next->is(tok::colon)))
-      return Style.AccessModifierOffset;
+         RootToken.Next && RootToken.Next->is(tok::colon))) {
+      // The AccessModifierOffset may be overriden by IndentAccessModifiers,
+      // in which case we take a negative value of the IndentWidth to simulate
+      // the upper indent level.
+      return Style.IndentAccessModifiers ? -Style.IndentWidth
+                                         : Style.AccessModifierOffset;
+    }
     return 0;
   }
 
   /// Get the indent of \p Level from \p IndentForLevel.
   ///
   /// \p IndentForLevel must contain the indent for the level \c l
   /// at \p IndentForLevel[l], or a value < 0 if the indent for
   /// that level is unknown.

clang/lib/Format/UnwrappedLineParser.h

UnwrappedLineParser(const FormatStyle &Style,
                       UnwrappedLineConsumer &Callback);
 
   void parse();
 
 private:
   void reset();
   void parseFile();
   void parseLevel(bool HasOpeningBrace);
-  void parseBlock(bool MustBeDeclaration, bool AddLevel = true,
+  void parseBlock(bool MustBeDeclaration, unsigned AddLevels = 1u,
                   bool MunchSemi = true);
   void parseChildBlock();
   void parsePPDirective(unsigned Level);
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();

clang/lib/Format/UnwrappedLineParser.cpp

size_t UnwrappedLineParser::computePPHash() const {
   size_t h = 0;
   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,
+void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, unsigned AddLevels,
                                      bool MunchSemi) {
   assert(FormatTok->isOneOf(tok::l_brace, TT_MacroBlockBegin) &&
          "'{' or macro block token expected");
   const bool MacroBlock = FormatTok->is(TT_MacroBlockBegin);
   FormatTok->setBlockKind(BK_Block);
 
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
-  nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
+  nextToken(/*LevelDifference=*/AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   size_t NbPreprocessorDirectives =
       CurrentLines == &Lines ? PreprocessorDirectives.size() : 0;
   addUnwrappedLine();
   size_t OpeningLineIndex =
       CurrentLines->empty()
           ? (UnwrappedLine::kInvalidIndex)
           : (CurrentLines->size() - 1 - NbPreprocessorDirectives);
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
                                           MustBeDeclaration);
-  if (AddLevel)
-    ++Line->Level;
+  Line->Level += AddLevels;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
     return;
 
   if (MacroBlock ? !FormatTok->is(TT_MacroBlockEnd)
                  : !FormatTok->is(tok::r_brace)) {
     Line->Level = InitialLevel;
     FormatTok->setBlockKind(BK_Block);
     return;
   }
 
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
-  nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
+  nextToken(/*LevelDifference=*/-AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
 
   if (FormatTok->is(tok::arrow)) {
     // Following the } we can find a trailing return type arrow
     // as part of an implicit conversion constraint.
     nextToken();
case tok::kw_extern:
     nextToken();
     if (FormatTok->Tok.is(tok::string_literal)) {
       nextToken();
       if (FormatTok->Tok.is(tok::l_brace)) {
         if (!Style.IndentExternBlock) {
           if (Style.BraceWrapping.AfterExternBlock) {
             addUnwrappedLine();
           }
-          parseBlock(/*MustBeDeclaration=*/true,
-                     /*AddLevel=*/Style.BraceWrapping.AfterExternBlock);
+          unsigned AddLevels = Style.BraceWrapping.AfterExternBlock ? 1u : 0u;
+          parseBlock(/*MustBeDeclaration=*/true, AddLevels);
         } else {
-          parseBlock(/*MustBeDeclaration=*/true,
-                     /*AddLevel=*/Style.IndentExternBlock ==
-                         FormatStyle::IEBS_Indent);
+          unsigned AddLevels =
+              Style.IndentExternBlock == FormatStyle::IEBS_Indent ? 1u : 0u;
+          parseBlock(/*MustBeDeclaration=*/true, AddLevels);
         }
         addUnwrappedLine();
         return;
       }
     }
     break;
   case tok::kw_export:
     if (Style.Language == FormatStyle::LK_JavaScript) {
case tok::kw_inline:
     }
     break;
   case tok::identifier:
     if (FormatTok->is(TT_ForEachMacro)) {
       parseForOrWhileLoop();
       return;
     }
     if (FormatTok->is(TT_MacroBlockBegin)) {
-      parseBlock(/*MustBeDeclaration=*/false, /*AddLevel=*/true,
+      parseBlock(/*MustBeDeclaration=*/false, /*AddLevels=*/1u,
                  /*MunchSemi=*/false);
       return;
     }
     if (FormatTok->is(Keywords.kw_import)) {
       if (Style.Language == FormatStyle::LK_JavaScript) {
         parseJavaScriptEs6ImportExport();
         return;
       }
while (FormatTok->isOneOf(tok::identifier, tok::coloncolon, tok::kw_inline,
       else
         nextToken();
     }
   }
   if (FormatTok->Tok.is(tok::l_brace)) {
     if (ShouldBreakBeforeBrace(Style, InitialToken))
       addUnwrappedLine();
 
-    bool AddLevel = Style.NamespaceIndentation == FormatStyle::NI_All ||
-                    (Style.NamespaceIndentation == FormatStyle::NI_Inner &&
-                     DeclarationScopeStack.size() > 1);
-    parseBlock(/*MustBeDeclaration=*/true, AddLevel);
+    unsigned AddLevels =
+        Style.NamespaceIndentation == FormatStyle::NI_All ||
+                 DeclarationScopeStack.size() > 1)
+            ? 1u
+            : 0u;
+    parseBlock(/*MustBeDeclaration=*/true, AddLevels);
     // Munch the semicolon after a namespace. This is more common than one would
     // think. Putting the semicolon into its own line is very ugly.
     if (FormatTok->Tok.is(tok::semi))
       nextToken();
     addUnwrappedLine();
   }
   // FIXME: Add error handling.
 }
void UnwrappedLineParser::parseJavaEnumBody() {
   nextToken();
   addUnwrappedLine();
   ++Line->Level;
 
   // Parse the enum constants.
   while (FormatTok) {
     if (FormatTok->is(tok::l_brace)) {
       // Parse the constant's class body.
-      parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true,
+      parseBlock(/*MustBeDeclaration=*/true, /*AddLevels=*/1u,
                  /*MunchSemi=*/false);
     } else if (FormatTok->is(tok::l_paren)) {
       parseParens();
     } else if (FormatTok->is(tok::comma)) {
       nextToken();
       addUnwrappedLine();
     } else if (FormatTok->is(tok::semi)) {
       nextToken();
void UnwrappedLineParser::parseRecord(bool ParseAsExpr) {
   }
   if (FormatTok->Tok.is(tok::l_brace)) {
     if (ParseAsExpr) {
       parseChildBlock();
     } else {
       if (ShouldBreakBeforeBrace(Style, InitialToken))
         addUnwrappedLine();
 
-      parseBlock(/*MustBeDeclaration=*/true, /*AddLevel=*/true,
-                 /*MunchSemi=*/false);
+      unsigned AddLevels = Style.IndentAccessModifiers ? 2u : 1u;
+      parseBlock(/*MustBeDeclaration=*/true, AddLevels, /*MunchSemi=*/false);
     }
   }
   // 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

TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
   CHECK_PARSE_BOOL(BreakStringLiterals);
   CHECK_PARSE_BOOL(CompactNamespaces);
   CHECK_PARSE_BOOL(ConstructorInitializerAllOnOneLineOrOnePerLine);
   CHECK_PARSE_BOOL(DeriveLineEnding);
   CHECK_PARSE_BOOL(DerivePointerAlignment);
   CHECK_PARSE_BOOL_FIELD(DerivePointerAlignment, "DerivePointerBinding");
   CHECK_PARSE_BOOL(DisableFormat);
+  CHECK_PARSE_BOOL(IndentAccessModifiers);
   CHECK_PARSE_BOOL(IndentCaseLabels);
   CHECK_PARSE_BOOL(IndentCaseBlocks);
   CHECK_PARSE_BOOL(IndentGotoLabels);
   CHECK_PARSE_BOOL(IndentPragmas);
   CHECK_PARSE_BOOL(IndentRequires);
   CHECK_PARSE_BOOL(IndentWrappedFunctionNames);
   CHECK_PARSE_BOOL(KeepEmptyLinesAtTheStartOfBlocks);
   CHECK_PARSE_BOOL(ObjCSpaceAfterProperty);
EXPECT_EQ("void foo() {\n"
                    "    for (int j = 0; j < 10; j++) {\n"
                    "    }\n"
                    "  }\n"
                    "#endif\n"
                    "}",
                    Style));
 }
 
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AccessModifierOffset = 4;
+  Style.AllowShortEnumsOnASingleLine = false;
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterEnum = false;
+  Style.IndentAccessModifiers = true;
+  verifyFormat("class C {\n"
+               "    int i;\n"
+               "};\n",
+               Style);
+  verifyFormat("class C {\n"
+               "  public:\n"
+               "    int i;\n"
+               "};\n",
+               Style);
+  {
+    const char *Expected = "struct S {\n"
+                           "  private:\n"
+                           "    class C {\n"
+                           "        int j;\n"
+                           "\n"
+                           "      public:\n"
+                           "        C();\n"
+                           "    };\n"
+                           "\n"
+                           "  public:\n"
+                           "    int i;\n"
+                           "};\n";
+    const char *ToFormat = "struct S {\n"
+                           "private:\n"
+                           "class C {\n"
+                           "int j;\n"
+                           "public:\n"
+                           "C();\n"
+                           "};\n"
+                           "public:\n"
+                           "int i;\n"
+                           "};\n";
+
+    EXPECT_EQ(Expected, format(ToFormat, Style));
+    EXPECT_EQ(Expected, format(Expected, Style));
+  }
+  verifyFormat("union C {\n"
+               "    int i;\n"
+               "    unsigned u;\n"
+               "};\n",
+               Style);
+  verifyFormat("enum class E {\n"
+               "  A,\n"
+               "  B\n"
+               "};\n",
+               Style);
+  verifyFormat("void foo() {\n"
+               "  class C {\n"
+               "      int i;\n"
+               "  };\n"
+               "  return;\n"
+               "}\n",
+               Style);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang