[clang-format] Add an option to control indentation of `export { ... }` #110381

Sirraide · 2024-09-28T18:52:31Z

export { ... } blocks can get a bit long, so I thought it would make sense to have an option that makes it so their contents are not indented (basically the same argument as for namespaces). This is my first contribution to clang-format (I normally contribute to Clang), so I’m not entirely sure if what I did is the best approach.

I based this on the NamespaceIndentation option, except that I didn’t include an option to control the behaviour of export blocks when nested because nesting them doesn’t really make sense (now that I think about it, iirc they could be nested inside something else though—e.g. a namespace—do we care about that case?).

llvmbot · 2024-09-28T18:52:53Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: None (Sirraide)

Changes

export { ... } blocks can get a bit long, so I thought it would make sense to have an option that makes it so their contents are not indented (basically the same argument as for namespaces). This is my first contribution to clang-format (I normally contribute to Clang), so I’m not entirely sure if what I did is the best approach.

I based this on the NamespaceIndentation option, except that I didn’t include an option to control the behaviour of export blocks when nested because nesting them doesn’t really make sense (now that I think about it, iirc they could be nested inside something else though—e.g. a namespace—do we care about that case?).

Full diff: https://github.com/llvm/llvm-project/pull/110381.diff

7 Files Affected:

(modified) clang/docs/ClangFormatStyleOptions.rst (+15)
(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/include/clang/Format/Format.h (+14)
(modified) clang/lib/Format/Format.cpp (+1)
(modified) clang/lib/Format/UnwrappedLineParser.cpp (+29-19)
(modified) clang/lib/Format/UnwrappedLineParser.h (+2)
(modified) clang/unittests/Format/FormatTest.cpp (+25)

diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index a427d7cd40fcdd..7bdb1d3383e056 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -3828,6 +3828,21 @@ the configuration (without a prefix: ``Auto``).
    This is an experimental flag, that might go away or be renamed. Do
    not use this in config files, etc. Use at your own risk.
 
+.. _ExportBlockIndentation:
+
+**ExportBlockIndentation** (``Boolean``) :versionbadge:`clang-format 20` :ref:`¶ <ExportBlockIndentation>`
+  If ``true``, clang-format will indent the body of an ``export { ... }``
+  block. This doesn't affect the formatting of anything else related to
+  exported declarations.
+
+  .. code-block:: c++
+
+     true:                     false:
+     export {          vs.     export {
+       void foo();             void foo();
+       void bar();             void bar();
+     }                         }
+
 .. _FixNamespaceComments:
 
 **FixNamespaceComments** (``Boolean``) :versionbadge:`clang-format 5` :ref:`¶ <FixNamespaceComments>`
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 14907e7db18de3..2588523e326d13 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -554,6 +554,7 @@ clang-format
 ------------
 
 - Adds ``BreakBinaryOperations`` option.
+- Adds the ``ExportBlockIndentation`` option.
 
 libclang
 --------
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index d8b62c7652a0f6..238a082e0f3670 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -2655,6 +2655,19 @@ struct FormatStyle {
   /// \version 3.7
   bool ExperimentalAutoDetectBinPacking;
 
+  /// If ``true``, clang-format will indent the body of an ``export { ... }``
+  /// block. This doesn't affect the formatting of anything else related to
+  /// exported declarations.
+  /// \code
+  ///    true:                     false:
+  ///    export {          vs.     export {
+  ///      void foo();             void foo();
+  ///      void bar();             void bar();
+  ///    }                         }
+  /// \endcode
+  /// \version 20
+  bool ExportBlockIndentation;
+
   /// If ``true``, clang-format adds missing namespace end comments for
   /// namespaces and fixes invalid existing ones. This doesn't affect short
   /// namespaces, which are controlled by ``ShortNamespaceLines``.
@@ -5131,6 +5144,7 @@ struct FormatStyle {
            EmptyLineBeforeAccessModifier == R.EmptyLineBeforeAccessModifier &&
            ExperimentalAutoDetectBinPacking ==
                R.ExperimentalAutoDetectBinPacking &&
+           ExportBlockIndentation == R.ExportBlockIndentation &&
            FixNamespaceComments == R.FixNamespaceComments &&
            ForEachMacros == R.ForEachMacros &&
            IncludeStyle.IncludeBlocks == R.IncludeStyle.IncludeBlocks &&
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index d2463b892fbb96..ee584b1ad0d1e5 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1016,6 +1016,7 @@ template <> struct MappingTraits<FormatStyle> {
                    Style.EmptyLineBeforeAccessModifier);
     IO.mapOptional("ExperimentalAutoDetectBinPacking",
                    Style.ExperimentalAutoDetectBinPacking);
+    IO.mapOptional("ExportBlockIndentation", Style.ExportBlockIndentation);
     IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
     IO.mapOptional("ForEachMacros", Style.ForEachMacros);
     IO.mapOptional("IfMacros", Style.IfMacros);
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 40f77266fabdca..306747290c0a7a 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -1616,6 +1616,10 @@ void UnwrappedLineParser::parseStructuralElement(
         parseNamespace();
         return;
       }
+      if (FormatTok->is(tok::l_brace)) {
+        parseCXXExportBlock();
+        return;
+      }
       if (FormatTok->is(Keywords.kw_import) && parseModuleImport())
         return;
     }
@@ -3075,6 +3079,26 @@ void UnwrappedLineParser::parseTryCatch() {
     addUnwrappedLine();
 }
 
+void UnwrappedLineParser::parseNamespaceOrExportBlock(unsigned AddLevels) {
+  bool ManageWhitesmithsBraces =
+      AddLevels == 0u && Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths;
+
+  // If we're in Whitesmiths mode, indent the brace if we're not indenting
+  // the whole block.
+  if (ManageWhitesmithsBraces)
+    ++Line->Level;
+
+  // Munch the semicolon after a namespace. This is more common than one would
+  // think. Putting the semicolon into its own line is very ugly.
+  parseBlock(/*MustBeDeclaration=*/true, AddLevels, /*MunchSemi=*/true,
+             /*KeepBraces=*/true, /*IfKind=*/nullptr, ManageWhitesmithsBraces);
+
+  addUnwrappedLine(AddLevels > 0 ? LineLevel::Remove : LineLevel::Keep);
+
+  if (ManageWhitesmithsBraces)
+    --Line->Level;
+}
+
 void UnwrappedLineParser::parseNamespace() {
   assert(FormatTok->isOneOf(tok::kw_namespace, TT_NamespaceMacro) &&
          "'namespace' expected");
@@ -3107,29 +3131,15 @@ void UnwrappedLineParser::parseNamespace() {
                  DeclarationScopeStack.size() > 1)
             ? 1u
             : 0u;
-    bool ManageWhitesmithsBraces =
-        AddLevels == 0u &&
-        Style.BreakBeforeBraces == FormatStyle::BS_Whitesmiths;
-
-    // If we're in Whitesmiths mode, indent the brace if we're not indenting
-    // the whole block.
-    if (ManageWhitesmithsBraces)
-      ++Line->Level;
-
-    // Munch the semicolon after a namespace. This is more common than one would
-    // think. Putting the semicolon into its own line is very ugly.
-    parseBlock(/*MustBeDeclaration=*/true, AddLevels, /*MunchSemi=*/true,
-               /*KeepBraces=*/true, /*IfKind=*/nullptr,
-               ManageWhitesmithsBraces);
-
-    addUnwrappedLine(AddLevels > 0 ? LineLevel::Remove : LineLevel::Keep);
-
-    if (ManageWhitesmithsBraces)
-      --Line->Level;
+    parseNamespaceOrExportBlock(AddLevels);
   }
   // FIXME: Add error handling.
 }
 
+void UnwrappedLineParser::parseCXXExportBlock() {
+  parseNamespaceOrExportBlock(Style.ExportBlockIndentation ? 1 : 0);
+}
+
 void UnwrappedLineParser::parseNew() {
   assert(FormatTok->is(tok::kw_new) && "'new' expected");
   nextToken();
diff --git a/clang/lib/Format/UnwrappedLineParser.h b/clang/lib/Format/UnwrappedLineParser.h
index b7daf8d9f44012..e5cfe4f6810791 100644
--- a/clang/lib/Format/UnwrappedLineParser.h
+++ b/clang/lib/Format/UnwrappedLineParser.h
@@ -171,6 +171,8 @@ class UnwrappedLineParser {
   void parseRequiresClause(FormatToken *RequiresToken);
   void parseRequiresExpression(FormatToken *RequiresToken);
   void parseConstraintExpression();
+  void parseCXXExportBlock();
+  void parseNamespaceOrExportBlock(unsigned AddLevels);
   void parseJavaEnumBody();
   // Parses a record (aka class) as a top level element. If ParseAsExpr is true,
   // parses the record as a child block, i.e. if the class declaration is an
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 5d386c1bbdbcd9..add52cd7460a48 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -9040,6 +9040,31 @@ TEST_F(FormatTest, AdaptiveOnePerLineFormatting) {
                Style);
 }
 
+TEST_F(FormatTest, ExportBlockIndentation) {
+  FormatStyle Style = getLLVMStyleWithColumns(80);
+  Style.ExportBlockIndentation = true;
+  verifyFormat("export {\n"
+               "  int x;\n"
+               "  int y;\n"
+               "}\n",
+               "export {\n"
+               "  int x;\n"
+               "  int y;\n"
+               "}\n",
+               Style);
+
+  Style.ExportBlockIndentation = false;
+  verifyFormat("export {\n"
+               "  int x;\n"
+               "  int y;\n"
+               "}\n",
+               "export {\n"
+               "int x;\n"
+               "int y;\n"
+               "}\n",
+               Style);
+}
+
 TEST_F(FormatTest, FormatsBuilderPattern) {
   verifyFormat("return llvm::StringSwitch<Reference::Kind>(name)\n"
                "    .StartsWith(\".eh_frame_hdr\", ORDER_EH_FRAMEHDR)\n"

mydeveloperday · 2024-10-03T07:20:53Z

clang/unittests/Format/FormatTest.cpp

-               "  int foo;\n"
-               "};",
-               Style);
+  verifyFormat("export { int foo; };", Style);


What's going on here?

You can't fix the tests by changing them... here you make the decision that people want a collapsed export {} what makes you say thats what everyone wants?

Yeah, makes sense. As far as I understand it, were were previously just parsing this as a compound statement, which afaik isn’t formatted on a single line by default, but maybe namespaces are, but I’m candidly not quite sure what’s causing this to be formatted on one line... I think this has something to do with the fact that I used parseBlock for this, but I’ll have to look into it a bit more.

Do we want a separate option for this (e.g. something like AllowShortExportBlocksOnASingleLine) or should that just fall under AllowShortBlocksOnASingleLine?

There should be an option to en/disable it, if it can be shared with other things, or be exclusively for export is to be discussed. I have no strong opinion, but would lean to an extra option, because someone will ask for it.

Makes sense; I’ll hopefully have time to look into this next week because I’ve been a bit busy recently.

HazardyKnusperkeks · 2024-10-24T18:25:10Z

clang/lib/Format/UnwrappedLineParser.cpp

  }
  // FIXME: Add error handling.
 }

+void UnwrappedLineParser::parseCXXExportBlock() {
+  parseNamespaceOrExportBlock(Style.ExportBlockIndentation ? 1 : 0);


Suggested change

parseNamespaceOrExportBlock(Style.ExportBlockIndentation ? 1 : 0);

parseNamespaceOrExportBlock(/*AddLevels=*/Style.ExportBlockIndentation ? 1 : 0);

HazardyKnusperkeks · 2024-10-24T18:29:50Z

clang/unittests/Format/FormatTest.cpp

-               "  int foo;\n"
-               "};",
-               Style);
+  verifyFormat("export { int foo; };", Style);


There should be an option to en/disable it, if it can be shared with other things, or be exclusively for export is to be discussed. I have no strong opinion, but would lean to an extra option, because someone will ask for it.

ChuanqiXu9 · 2025-01-06T08:03:06Z

@Sirraide hi are you still working on this? We observed similar issue recently.

Sirraide added 2 commits September 28, 2024 20:37

[clang-format] Add an option to control indentation of export { ... }

570aff7

Add release note

21b0563

Sirraide added the clang-format label Sep 28, 2024

Sirraide requested review from owenca, mydeveloperday and HazardyKnusperkeks September 28, 2024 18:52

llvmbot added the clang Clang issues not falling into any other category label Sep 28, 2024

Fix tests

c53fb38

mydeveloperday requested changes Oct 4, 2024

View reviewed changes

HazardyKnusperkeks reviewed Oct 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-format] Add an option to control indentation of `export { ... }` #110381

[clang-format] Add an option to control indentation of `export { ... }` #110381

Sirraide commented Sep 28, 2024 •

edited by owenca

Loading

llvmbot commented Sep 28, 2024 •

edited

Loading

mydeveloperday Oct 3, 2024

mydeveloperday Oct 4, 2024

Sirraide Oct 8, 2024

HazardyKnusperkeks Oct 24, 2024

Sirraide Oct 25, 2024

HazardyKnusperkeks Oct 24, 2024

HazardyKnusperkeks Oct 24, 2024

ChuanqiXu9 commented Jan 6, 2025

	parseNamespaceOrExportBlock(Style.ExportBlockIndentation ? 1 : 0);
	parseNamespaceOrExportBlock(/AddLevels=/Style.ExportBlockIndentation ? 1 : 0);

[clang-format] Add an option to control indentation of export { ... } #110381

Are you sure you want to change the base?

[clang-format] Add an option to control indentation of export { ... } #110381

Conversation

Sirraide commented Sep 28, 2024 • edited by owenca Loading

llvmbot commented Sep 28, 2024 • edited Loading

mydeveloperday Oct 3, 2024

Choose a reason for hiding this comment

mydeveloperday Oct 4, 2024

Choose a reason for hiding this comment

Sirraide Oct 8, 2024

Choose a reason for hiding this comment

HazardyKnusperkeks Oct 24, 2024

Choose a reason for hiding this comment

Sirraide Oct 25, 2024

Choose a reason for hiding this comment

HazardyKnusperkeks Oct 24, 2024

Choose a reason for hiding this comment

HazardyKnusperkeks Oct 24, 2024

Choose a reason for hiding this comment

ChuanqiXu9 commented Jan 6, 2025

[clang-format] Add an option to control indentation of `export { ... }` #110381

[clang-format] Add an option to control indentation of `export { ... }` #110381

Sirraide commented Sep 28, 2024 •

edited by owenca

Loading

llvmbot commented Sep 28, 2024 •

edited

Loading