[clang-format] Add new option: WrapNamespaceBodyWithNewlines #106145

dmasloff · 2024-08-26T21:32:51Z

I would like to suggest a new clang-format option for llvm-project - WrapNamespaceBodyWithNewlines. I think it can be added to upstream since it is used by many popular public repositories, for example, ytsaurus. You can look through their style guide at this page: https://github.com/ytsaurus/ytsaurus/blob/main/yt/styleguide/cpp.md#namespaces

As you can see from the name of the option it wraps the body of namespace with additional newlines turning this code:

namespace N {
int function();
}

into that:

namespace N {

int function();

}

Looking forward to your advices and recommendations

github-actions · 2024-08-26T21:33:08Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-08-26T21:33:36Z

@llvm/pr-subscribers-clang-format

@llvm/pr-subscribers-clang

Author: None (dmasloff)

Changes

I would like to suggest a new clang-format option for llvm-project - WrapNamespaceBodyWithNewlines. I think it can be added to upstream since it is used by many popular public repositories, for example, ytsaurus. You can look through their style guide at this page: https://github.com/ytsaurus/ytsaurus/blob/main/yt/styleguide/cpp.md#namespaces

As you can see from the name of the option it wraps the body of namespace with additional newlines turning this code:

namespace N {
int function();
}

into that:

namespace N {

int function();

}

Looking forward to your advices and recommendations

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

5 Files Affected:

(modified) clang/docs/ClangFormatStyleOptions.rst (+17)
(modified) clang/include/clang/Format/Format.h (+17-1)
(modified) clang/lib/Format/Format.cpp (+3)
(modified) clang/lib/Format/UnwrappedLineFormatter.cpp (+30)
(modified) clang/unittests/Format/FormatTest.cpp (+73)

diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index a427d7cd40fcdd..140787ad9776d9 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -6652,6 +6652,23 @@ the configuration (without a prefix: ``Auto``).
 
   For example: BOOST_PP_STRINGIZE
 
+.. _WrapNamespaceBodyWithNewlines:
+
+**WrapNamespaceBodyWithNewlines** (``Boolean``) :versionbadge:`clang-format 19` :ref:`¶ <WrapNamespaceBodyWithNewlines>`
+  Insert a newline at the begging and at the end of namespace definition
+
+  .. code-block:: c++
+
+    false:                           vs.      true:
+
+    namespace a {                             namespace a {
+    namespace b {                             namespace b {
+      function();
+    }                                         function();
+    }
+                                              }
+                                              }
+
 .. END_FORMAT_STYLE_OPTIONS
 
 Adding additional style options
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index d8b62c7652a0f6..eb8f198cc8a919 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -5057,6 +5057,21 @@ struct FormatStyle {
   /// \version 11
   std::vector<std::string> WhitespaceSensitiveMacros;
 
+  /// Insert a newline at the begging and at the end of namespace definition
+  /// \code
+  ///   false:                           vs.      true:
+  ///
+  ///   namespace a {                             namespace a {
+  ///   namespace b {                             namespace b {
+  ///     function();
+  ///   }                                         function();
+  ///   }
+  ///                                             }
+  ///                                             }
+  /// \endcode
+  /// \version 19
+  bool WrapNamespaceBodyWithNewlines;
+
   bool operator==(const FormatStyle &R) const {
     return AccessModifierOffset == R.AccessModifierOffset &&
            AlignAfterOpenBracket == R.AlignAfterOpenBracket &&
@@ -5234,7 +5249,8 @@ struct FormatStyle {
            TypenameMacros == R.TypenameMacros && UseTab == R.UseTab &&
            VerilogBreakBetweenInstancePorts ==
                R.VerilogBreakBetweenInstancePorts &&
-           WhitespaceSensitiveMacros == R.WhitespaceSensitiveMacros;
+           WhitespaceSensitiveMacros == R.WhitespaceSensitiveMacros &&
+           WrapNamespaceBodyWithNewlines == R.WrapNamespaceBodyWithNewlines;
   }
 
   std::optional<FormatStyle> GetLanguageStyle(LanguageKind Language) const;
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index d2463b892fbb96..65d7737b397212 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -1154,6 +1154,8 @@ template <> struct MappingTraits<FormatStyle> {
                    Style.VerilogBreakBetweenInstancePorts);
     IO.mapOptional("WhitespaceSensitiveMacros",
                    Style.WhitespaceSensitiveMacros);
+    IO.mapOptional("WrapNamespaceBodyWithNewlines",
+                   Style.WrapNamespaceBodyWithNewlines);
 
     // If AlwaysBreakAfterDefinitionReturnType was specified but
     // BreakAfterReturnType was not, initialize the latter from the former for
@@ -1623,6 +1625,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.WhitespaceSensitiveMacros.push_back("NS_SWIFT_NAME");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("PP_STRINGIZE");
   LLVMStyle.WhitespaceSensitiveMacros.push_back("STRINGIZE");
+  LLVMStyle.WrapNamespaceBodyWithNewlines = false;
 
   LLVMStyle.PenaltyBreakAssignment = prec::Assignment;
   LLVMStyle.PenaltyBreakBeforeFirstCallParameter = 19;
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..f3f76ac227df50 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -32,6 +32,24 @@ bool isRecordLBrace(const FormatToken &Tok) {
                      TT_StructLBrace, TT_UnionLBrace);
 }
 
+bool LineStartsNamespaceScope(const AnnotatedLine* Line,
+                              const AnnotatedLine* PreviousLine,
+                              const AnnotatedLine* PrevPrevLine) {
+  return PreviousLine && PreviousLine->Last->is(tok::l_brace) &&
+         (PreviousLine->startsWithNamespace() ||
+          (PrevPrevLine && PrevPrevLine->startsWithNamespace() &&
+           PreviousLine->startsWith(tok::l_brace))) && !Line->startsWithNamespace();
+}
+
+bool LineEndsNamespaceScope(const AnnotatedLine *Line,
+                            const SmallVectorImpl<AnnotatedLine*> &Lines) {
+  const FormatToken* tok = Line->First;
+  if (!tok || tok->isNot(tok::r_brace)) {
+    return false;
+  }
+  return getNamespaceToken(Line, Lines) != nullptr;
+}
+
 /// Tracks the indent level of \c AnnotatedLines across levels.
 ///
 /// \c nextLine must be called for each \c AnnotatedLine, after which \c
@@ -1493,6 +1511,18 @@ static auto computeNewlines(const AnnotatedLine &Line,
     Newlines = 1;
   }
 
+  // Insert empty line after "{" that opens namespace scope
+  if (Style.WrapNamespaceBodyWithNewlines &&
+      LineStartsNamespaceScope(&Line, PreviousLine, PrevPrevLine)) {
+    Newlines = 2;
+  }
+
+  // Insert empty line before "}" that closes namespace scope
+  if (Style.WrapNamespaceBodyWithNewlines &&
+      LineEndsNamespaceScope(&Line, Lines) && !LineEndsNamespaceScope(PreviousLine, Lines)) {
+    Newlines = std::max(Newlines, 2u);
+  }
+
   // Insert or remove empty line before access specifiers.
   if (PreviousLine && RootToken.isAccessSpecifier()) {
     switch (Style.EmptyLineBeforeAccessModifier) {
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index a383a624434b1f..94d8d652f80046 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28084,6 +28084,79 @@ TEST_F(FormatTest, BreakBinaryOperations) {
                Style);
 }
 
+TEST_F(FormatTest, WrappingNamespaceBodyWithNewlines) {
+  FormatStyle Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Style.ShortNamespaceLines = 0;
+  Style.WrapNamespaceBodyWithNewlines = true;
+
+  // Empty namespace
+  verifyFormat("namespace N {};", Style);
+
+  // Single namespace
+  verifyFormat("namespace N {\n\n"
+               "int f1(int a) { return 2 * a; }\n\n"
+               "};", Style);
+
+  // Nested namespace
+  verifyFormat("namespace N1 {\n"
+               "namespace N2 {\n"
+               "namespace N3 {\n\n"
+               "int f1() {\n"
+               "  int a = 1;\n"
+               "  return a;\n"
+               "}\n\n"
+               "}\n"
+               "}\n"
+               "}", Style);
+
+  Style.WrapNamespaceBodyWithNewlines = false;
+
+  // Empty namespace
+  verifyFormat("namespace N {};", Style);
+
+  // Single namespace
+  verifyFormat("namespace N {\n"
+               "int f1(int a) { return 2 * a; }\n"
+               "};", Style);
+
+  // Nested namespace
+  verifyFormat("namespace N1 {\n"
+               "namespace N2 {\n"
+               "namespace N3 {\n"
+               "int f1() {\n"
+               "  int a = 1;\n"
+               "  return a;\n"
+               "}\n"
+               "}\n"
+               "}\n"
+               "}", Style);
+}
+
+TEST_F(FormatTest, WrappingNamespaceBodyWithNewlinesWithCompactNamespaces) {
+  FormatStyle Style = getLLVMStyle();
+  Style.FixNamespaceComments = false;
+  Style.CompactNamespaces = true;
+  Style.WrapNamespaceBodyWithNewlines = true;
+
+
+  verifyFormat("namespace N1 { namespace N2 { namespace N3 {\n\n"
+               "int f1() {\n"
+               "  int a = 1;\n"
+               "  return a;\n"
+               "}\n\n"
+               "}}}", Style);
+
+  Style.WrapNamespaceBodyWithNewlines = false;
+
+  verifyFormat("namespace N1 { namespace N2 { namespace N3 {\n"
+               "int f1() {\n"
+               "  int a = 1;\n"
+               "  return a;\n"
+               "}\n"
+               "}}}", Style);
+}
+
 } // namespace
 } // namespace test
 } // namespace format

dmasloff · 2024-08-27T08:47:33Z

@mydeveloperday sorry for tagging, I hope it could save some time for out team as in this issue you seemed positive about new contributors

github-actions · 2024-08-27T18:02:57Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clang/include/clang/Format/Format.h

clang/lib/Format/UnwrappedLineFormatter.cpp

owenca · 2024-09-01T01:19:09Z

Please run git clang-format HEAD~ and ninja clang-format-check-format before pushing.

dmasloff · 2024-09-01T09:32:25Z

Please run git clang-format HEAD~ and ninja clang-format-check-format before pushing.

Maybe it's only my problem, but i get this message running git clang-format HEAD~, hopefully I could fix it all manually using ninja clang-format-check-format:

/Users/daniilmaslov/llvm-project-copy/clang/include/clang/Format/.clang-format:1:1: error: Unknown value for BasedOnStyle: clang-format
BasedOnStyle: clang-format
^
Error reading /Users/daniilmaslov/llvm-project-copy/clang/include/clang/Format/.clang-format: Invalid argument
error: `clang-format -lines=5060:5096 -lines=5274:5275 clang/include/clang/Format/Format.h` failed

clang/unittests/Format/FormatTest.cpp

clang/docs/ClangFormatStyleOptions.rst

clang/unittests/Format/FormatTest.cpp

clang/include/clang/Format/Format.h

clang/lib/Format/Format.cpp

clang/lib/Format/UnwrappedLineFormatter.cpp

owenca

Running FormatTests failed:

[ RUN      ] FormatTest.WrapNamespaceBodyWithEmptyLinesNever
Assertion failed: (TheLine->MatchingClosingBlockLineIndex > 0), function tryFitMultipleLinesInOne, file UnwrappedLineFormatter.cpp, line 390.

dmasloff · 2024-09-17T15:25:12Z

Running FormatTests failed:

[ RUN      ] FormatTest.WrapNamespaceBodyWithEmptyLinesNever
Assertion failed: (TheLine->MatchingClosingBlockLineIndex > 0), function tryFitMultipleLinesInOne, file UnwrappedLineFormatter.cpp, line 390.

@owenca I looked through this error, fortunately it is not connected with code changes introduced in my pr, but with additional test, which I wrote. Formatting this test assert fails both in my code and in llvm-project/main. The problem occurs in this test during formatting of the empty namespace that stands right in the beginning of file with CompactNamespaces: true option, because TheLine->MatchingClosingBlockLineIndex in this case equals 0 as file consists of just one line. I will remove this test from my pr

clang/unittests/Format/FormatTest.cpp

dmasloff · 2024-10-10T13:00:21Z

@owenca @mydeveloperday @HazardyKnusperkeks maybe somebody could continue the review?

owenca

Please update the release notes.

clang/lib/Format/UnwrappedLineFormatter.cpp

clang/unittests/Format/FormatTest.cpp

clang/lib/Format/UnwrappedLineFormatter.cpp

clang/unittests/Format/FormatTest.cpp

owenca

Please enable "Allowing edits by maintainers".

dmasloff · 2025-01-01T00:26:55Z

Resolved merge conflict

owenca

Also, please address #106145 (review).

clang/unittests/Format/FormatTest.cpp

Add missing dependency that sometimes makes a build fails with ninja.

github-actions · 2025-01-03T05:52:19Z

@dmasloff Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added clang Clang issues not falling into any other category clang-format labels Aug 26, 2024

HazardyKnusperkeks reviewed Aug 27, 2024

View reviewed changes

dmasloff force-pushed the clang-format-option-wrap-namespace-with-breaks branch from 84ba993 to 568c99f Compare August 31, 2024 12:46

dmasloff requested a review from HazardyKnusperkeks August 31, 2024 13:18

dmasloff force-pushed the clang-format-option-wrap-namespace-with-breaks branch from 568c99f to 219424f Compare September 1, 2024 09:06

HazardyKnusperkeks reviewed Sep 2, 2024

View reviewed changes

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

dmasloff requested a review from HazardyKnusperkeks September 2, 2024 15:02

dmasloff force-pushed the clang-format-option-wrap-namespace-with-breaks branch from 8c34acb to bf76cbe Compare September 2, 2024 20:18

mydeveloperday requested changes Sep 3, 2024

View reviewed changes

clang/lib/Format/UnwrappedLineFormatter.cpp Outdated Show resolved Hide resolved

mydeveloperday requested a review from owenca September 3, 2024 11:41

dmasloff requested a review from mydeveloperday September 4, 2024 05:53

owenca requested changes Sep 9, 2024

View reviewed changes

dmasloff requested a review from owenca September 17, 2024 17:41

owenca reviewed Sep 22, 2024

View reviewed changes

dmasloff requested a review from owenca September 26, 2024 06:37

owenca reviewed Oct 14, 2024

View reviewed changes

owenca reviewed Dec 31, 2024

View reviewed changes

clang/lib/Format/UnwrappedLineFormatter.cpp Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

owenca requested changes Dec 31, 2024

View reviewed changes

dmasloff force-pushed the clang-format-option-wrap-namespace-with-breaks branch from af6ef19 to 94509ed Compare January 1, 2025 00:26

dmasloff requested a review from owenca January 1, 2025 00:27

owenca reviewed Jan 2, 2025

View reviewed changes

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

dmasloff and others added 9 commits January 2, 2025 22:01

fix merge conflict

dde31f1

FormatTests update

fd58d2f

ConfigParseTest added for new option + minor code-style fixes

9134678

Fix invalid tests with CompactNamespaces option

3574ea9

Reformat tests in FormatTest.cpp

f361664

Remove reduntant tests and simplify code

061f409

minor fix

78c1d98

[flang][cuda] Add missing dependency (#106298)

7f07b52

Add missing dependency that sometimes makes a build fails with ninja.

Adds review suggestions + adds option to clang/docs/ReleaseNotes.rst

1edc890

dmasloff force-pushed the clang-format-option-wrap-namespace-with-breaks branch from ebe57d3 to 1edc890 Compare January 2, 2025 19:07

dmasloff requested a review from owenca January 2, 2025 21:43

owenca approved these changes Jan 3, 2025

View reviewed changes

owenca added 3 commits January 2, 2025 20:50

Clean up documentation

228747f

Update ClangFormatStyleOptions.rst

87337ba

Delete a redundant assignment in ConfigParseTest.cpp

78dfcb9

owenca merged commit 1c997fe into llvm:main Jan 3, 2025
9 checks passed

owenca removed the clang Clang issues not falling into any other category label Jan 3, 2025

[clang-format] Add new option: WrapNamespaceBodyWithNewlines #106145

[clang-format] Add new option: WrapNamespaceBodyWithNewlines #106145

Uh oh!

Conversation

dmasloff commented Aug 26, 2024

Uh oh!

github-actions bot commented Aug 26, 2024

Uh oh!

llvmbot commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmasloff commented Aug 27, 2024

Uh oh!

github-actions bot commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenca commented Sep 1, 2024

Uh oh!

dmasloff commented Sep 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

dmasloff commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmasloff commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

dmasloff commented Jan 1, 2025

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 3, 2025

Uh oh!

Uh oh!

llvmbot commented Aug 26, 2024 •

edited

Loading

github-actions bot commented Aug 27, 2024 •

edited

Loading

dmasloff commented Sep 17, 2024 •

edited

Loading

dmasloff commented Oct 10, 2024 •

edited

Loading