[clang-format] [PR19056] Add support for access modifiers indentation
authorJakub Budiský <jakub.budisky@gmail.com>
Fri, 26 Feb 2021 08:05:35 +0000 (09:05 +0100)
committerMarek Kurdej <marek.kurdej@gmail.com>
Fri, 26 Feb 2021 08:17:07 +0000 (09:17 +0100)
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.

Reviewed By: MyDeveloperDay, curdeius, HazardyKnusperkeks

Differential Revision: https://reviews.llvm.org/D94661

clang/docs/ClangFormatStyleOptions.rst
clang/docs/ReleaseNotes.rst
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/lib/Format/UnwrappedLineParser.cpp
clang/lib/Format/UnwrappedLineParser.h
clang/unittests/Format/FormatTest.cpp

index 7ae8ea9..1dbf119 100644 (file)
@@ -2360,6 +2360,33 @@ the configuration (without a prefix: ``Auto``).
   ``ClassImpl.hpp`` would not have the main include file put on top
   before any other include.
 
+**IndentAccessModifiers** (``bool``)
+  Specify whether access modifiers should have their own indentation level.
+
+  When ``false``, access modifiers are indented (or outdented) relative to
+  the record members, respecting the ``AccessModifierOffset``. Record
+  members are indented one level below the record.
+  When ``true``, access modifiers get their own indentation level. As a
+  consequence, record members are indented 2 levels below the record,
+  regardless of the access modifier presence. Value of the
+  ``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.
 
index 26b3804..6a0ebd9 100644 (file)
@@ -196,6 +196,9 @@ clang-format
 - ``BasedOnStyle: InheritParentConfig`` allows to use the ``.clang-format`` of
   the parent directories to overwrite only parts of it.
 
+- Option ``IndentAccessModifiers`` has been added to be able to give access
+  modifiers their own indentation level inside records.
+
 libclang
 --------
 
index 1a669eb..d60c560 100644 (file)
@@ -2047,6 +2047,32 @@ struct FormatStyle {
 
   tooling::IncludeStyle IncludeStyle;
 
+  /// Specify whether access modifiers should have their own indentation level.
+  ///
+  /// When ``false``, access modifiers are indented (or outdented) relative to
+  /// the record members, respecting the ``AccessModifierOffset``. Record
+  /// members are indented one level below the record.
+  /// When ``true``, access modifiers get their own indentation level. As a
+  /// consequence, record members are always indented 2 levels below the record,
+  /// regardless of the access modifier presence. Value of the
+  /// ``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
@@ -3161,6 +3187,7 @@ struct FormatStyle {
                R.IncludeStyle.IncludeIsMainRegex &&
            IncludeStyle.IncludeIsMainSourceRegex ==
                R.IncludeStyle.IncludeIsMainSourceRegex &&
+           IndentAccessModifiers == R.IndentAccessModifiers &&
            IndentCaseLabels == R.IndentCaseLabels &&
            IndentCaseBlocks == R.IndentCaseBlocks &&
            IndentGotoLabels == R.IndentGotoLabels &&
index a8808b3..f2fc098 100644 (file)
@@ -597,6 +597,7 @@ template <> struct MappingTraits<FormatStyle> {
     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);
@@ -984,6 +985,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
       {".*", 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;
index 5dd0ccd..0cdf20f 100644 (file)
@@ -101,8 +101,13 @@ private:
     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;
   }
 
index f689a63..1e70da8 100644 (file)
@@ -579,7 +579,7 @@ size_t UnwrappedLineParser::computePPHash() const {
   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");
@@ -589,7 +589,7 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
   size_t PPStartHash = computePPHash();
 
   unsigned InitialLevel = Line->Level;
-  nextToken(/*LevelDifference=*/AddLevel ? 1 : 0);
+  nextToken(/*LevelDifference=*/AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
@@ -604,8 +604,7 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
 
   ScopedDeclarationState DeclarationState(*Line, DeclarationScopeStack,
                                           MustBeDeclaration);
-  if (AddLevel)
-    ++Line->Level;
+  Line->Level += AddLevels;
   parseLevel(/*HasOpeningBrace=*/true);
 
   if (eof())
@@ -621,7 +620,7 @@ void UnwrappedLineParser::parseBlock(bool MustBeDeclaration, bool AddLevel,
   size_t PPEndHash = computePPHash();
 
   // Munch the closing brace.
-  nextToken(/*LevelDifference=*/AddLevel ? -1 : 0);
+  nextToken(/*LevelDifference=*/-AddLevels);
 
   if (MacroBlock && FormatTok->is(tok::l_paren))
     parseParens();
@@ -1125,12 +1124,12 @@ void UnwrappedLineParser::parseStructuralElement() {
           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;
@@ -1159,7 +1158,7 @@ void UnwrappedLineParser::parseStructuralElement() {
       return;
     }
     if (FormatTok->is(TT_MacroBlockBegin)) {
-      parseBlock(/*MustBeDeclaration=*/false, /*AddLevel=*/true,
+      parseBlock(/*MustBeDeclaration=*/false, /*AddLevels=*/1u,
                  /*MunchSemi=*/false);
       return;
     }
@@ -2128,10 +2127,13 @@ void UnwrappedLineParser::parseNamespace() {
     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 ||
+                (Style.NamespaceIndentation == FormatStyle::NI_Inner &&
+                 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))
@@ -2577,7 +2579,7 @@ void UnwrappedLineParser::parseJavaEnumBody() {
   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();
@@ -2679,8 +2681,8 @@ void UnwrappedLineParser::parseRecord(bool ParseAsExpr) {
       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
index 02b328c..fde9e07 100644 (file)
@@ -85,7 +85,7 @@ 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();
index ccda0a8..b4b31fa 100644 (file)
@@ -15453,6 +15453,7 @@ TEST_F(FormatTest, ParsesConfigurationBools) {
   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);
@@ -19155,6 +19156,57 @@ TEST_F(FormatTest, StatementAttributeLikeMacros) {
             "}",
             format(Source, Style));
 }
+
+TEST_F(FormatTest, IndentAccessModifiers) {
+  FormatStyle Style = getLLVMStyle();
+  Style.IndentAccessModifiers = true;
+  // Members are *two* levels below the record;
+  // Style.IndentWidth == 2, thus yielding a 4 spaces wide indentation.
+  verifyFormat("class C {\n"
+               "    int i;\n"
+               "};\n",
+               Style);
+  verifyFormat("union C {\n"
+               "    int i;\n"
+               "    unsigned u;\n"
+               "};\n",
+               Style);
+  // Access modifiers should be indented one level below the record.
+  verifyFormat("class C {\n"
+               "  public:\n"
+               "    int i;\n"
+               "};\n",
+               Style);
+  verifyFormat("struct S {\n"
+               "  private:\n"
+               "    class C {\n"
+               "        int j;\n"
+               "\n"
+               "      public:\n"
+               "        C();\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"
+               "  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"
+               "   public:\n"
+               "      int i;\n"
+               "};\n",
+               Style);
+}
 } // namespace
 } // namespace format
 } // namespace clang