from:"Galen Elias via cfe\-commits"

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-05 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 1/4] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-09 Thread Galen Elias via cfe-commits



@@ -27981,6 +27981,118 @@ TEST_F(FormatTest, BreakBinaryOperations) {
Style);
 }
 
+TEST_F(FormatTest, ShortNamespacesOption) {
+  FormatStyle Style = getLLVMStyle();
+  Style.AllowShortNamespacesOnASingleLine = true;
+  Style.FixNamespaceComments = false;
+
+  // Basic functionality.
+  verifyFormat("namespace foo { class bar; }", Style);
+  verifyFormat("namespace foo::bar { class baz; }", Style);
+  verifyFormat("namespace { class bar; }", Style);
+  verifyFormat("namespace foo {\n"
+   "class bar;\n"
+   "class baz;\n"
+   "}",
+   Style);
+
+  // Trailing comments prevent merging.
+  verifyFormat("namespace foo {\n"
+   "namespace baz { class qux; } // comment\n"
+   "}",
+   Style);
+
+  // Make sure code doesn't walk to far on unbalanced code.
+  verifyFormat("namespace foo {", Style);
+  verifyFormat("namespace foo {\n"
+   "class baz;",
+   Style);
+  verifyFormat("namespace foo {\n"
+   "namespace bar { class baz; }",
+   Style);
+
+  // Nested namespaces.
+  verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
+  verifyFormat("namespace foo {\n"
+   "namespace bar { class baz; }\n"
+   "namespace quar { class quaz; }\n"
+   "}",
+   Style);
+
+  // Varying inner content.
+  verifyFormat("namespace foo {\n"
+   "int f() { return 5; }\n"
+   "}",
+   Style);
+  verifyFormat("namespace foo { template  struct bar; }", Style);
+  verifyFormat("namespace foo { constexpr int num = 42; }", Style);
+
+  // Validate wrapping scenarios around the ColumnLimit.
+  Style.ColumnLimit = 64;
+
+  // Validate just under the ColumnLimit.
+  verifyFormat(
+  "namespace foo { namespace bar { namespace baz { class qux; } } }",
+  Style);
+
+  // Validate just over the ColumnLimit.
+  verifyFormat("namespace foo {\n"
+   "namespace bar { namespace baz { class quux; } }\n"
+   "}",
+   Style);
+
+  verifyFormat("namespace foo {\n"
+   "namespace bar {\n"
+   "namespace baz { namespace qux { class quux; } }\n"
+   "}\n"
+   "}",
+   Style);
+
+  // Validate that the ColumnLimit logic accounts for trailing content as well.
+  verifyFormat("namespace foo {\n"
+   "namespace bar { namespace baz { class qux; } }\n"
+   "} // extra",
+   Style);
+
+  // No ColumnLimit, allows long nested one-liners, but also leaves multi-line
+  // instances alone.
+  Style.ColumnLimit = 0;
+  verifyFormat("namespace foo { namespace bar { namespace baz { namespace qux "
+   "{ class quux; } } } }",

galenelias wrote:

The clang-formatting rules seem to preclude this.  It will always pull the code 
into what I have here, which is having the string split across two lines and 
starting on the line with the verifyFormat.

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-09 Thread Galen Elias via cfe-commits


https://github.com/galenelias deleted 
https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-08-28 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 1/3] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-08-21 Thread Galen Elias via cfe-commits


https://github.com/galenelias created 
https://github.com/llvm/llvm-project/pull/105597

This addresses: https://github.com/llvm/llvm-project/issues/101363 which is a 
resurrection of a previously opened but never completed review: 
https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across 
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are ideally 
kept compact. This is also apparently the format that include-what-you-use will 
insert for forward declarations.

>From 3819c15c8c6258d1d29a38fcfb71db8567fb34e5 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 15:46:35 -0700
Subject: [PATCH] clang-format: Add "AllowShortNamespacesOnASingleLine" option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index fea5d2e17a0e28..9d72b82549afc2 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 7fd42e46e0ccb7..1272aed7e9b042 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -941,6 +941,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1445,6 +1447,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limit

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-08-21 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 3819c15c8c6258d1d29a38fcfb71db8567fb34e5 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 15:46:35 -0700
Subject: [PATCH] clang-format: Add "AllowShortNamespacesOnASingleLine" option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index fea5d2e17a0e28..9d72b82549afc2 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 7fd42e46e0ccb7..1272aed7e9b042 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -941,6 +941,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1445,6 +1447,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+r

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-08-21 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH] clang-format: Add "AllowShortNamespacesOnASingleLine" option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+r

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-08-22 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 1/2] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-10-08 Thread Galen Elias via cfe-commits


galenelias wrote:

Ping

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-10-22 Thread Galen Elias via cfe-commits


galenelias wrote:

> I don't think we want to add any tests whose formats are questionable or 
> still under discussion. WDYT @mydeveloperday @HazardyKnusperkeks @rymiel ?

Ok, that's fair.  I have removed these tests.  Thanks for the detailed response 
and context.

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-10-22 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 01/11] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-11-08 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 01/12] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-11-12 Thread Galen Elias via cfe-commits


galenelias wrote:

Ping

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-25 Thread Galen Elias via cfe-commits



@@ -616,6 +626,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)

galenelias wrote:

We can't take an `int` here, because when ColumnLimit = 0, then the calling 
code passes in `UINT_MAX` which would then underflow the int.  I could 
technically take a int64_t, but in general it seems like the style is to 
pre-check for underflow before doing subtraction, which is the approach here 
(although it's a little indirect).  See below.

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-25 Thread Galen Elias via cfe-commits



@@ -616,6 +626,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }".
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  const unsigned InnerLimit = Limit - I[1]->Last->TotalLength - 3;
+  const unsigned MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+  if (!MergedLines)

galenelias wrote:

This underflow is prevented by prior call to `nextTwoLinesFitInto`, which will 
ensure that Limit is large enough such that this won't underflow.

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-25 Thread Galen Elias via cfe-commits



@@ -616,6 +626,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }".
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))

galenelias wrote:

Yah, fair question.  I think I could see it either way.  I'm fine starting with 
not supporting trailing block comments either and then could relax it if there 
are explicit requests for that behavior.

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-25 Thread Galen Elias via cfe-commits



@@ -27981,6 +27981,132 @@ TEST_F(FormatTest, BreakBinaryOperations) {
Style);
 }
 
+TEST_F(FormatTest, ShortNamespacesOption) {
+  auto BaseStyle = getLLVMStyle();
+  BaseStyle.AllowShortNamespacesOnASingleLine = true;
+  BaseStyle.FixNamespaceComments = false;
+
+  auto Style = BaseStyle;
+
+  // Basic functionality.
+  verifyFormat("namespace foo { class bar; }", Style);
+  verifyFormat("namespace foo::bar { class baz; }", Style);
+  verifyFormat("namespace { class bar; }", Style);
+  verifyFormat("namespace foo {\n"
+   "class bar;\n"
+   "class baz;\n"
+   "}",
+   Style);
+
+  // Trailing comments prevent merging.
+  verifyFormat("namespace foo {\n"
+   "namespace baz { class qux; } // comment\n"
+   "}",
+   Style);
+
+  // Make sure code doesn't walk too far on unbalanced code.
+  verifyFormat("namespace foo {", Style);
+  verifyFormat("namespace foo {\n"
+   "class baz;",
+   Style);
+  verifyFormat("namespace foo {\n"
+   "namespace bar { class baz; }",
+   Style);
+
+  // Nested namespaces.
+  verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
+  verifyFormat("namespace foo {\n"
+   "namespace bar { class baz; }\n"
+   "namespace qux { class quux; }\n"
+   "}",
+   Style);
+
+  // Varying inner content.
+  verifyFormat("namespace foo {\n"
+   "int f() { return 5; }\n"
+   "}",
+   Style);
+  verifyFormat("namespace foo { template  struct bar; }", Style);
+  verifyFormat("namespace foo { constexpr int num = 42; }", Style);
+
+  // Validate wrapping scenarios around the ColumnLimit.
+  Style.ColumnLimit = 64;
+
+  // Validate just under the ColumnLimit.
+  verifyFormat(
+  "namespace foo { namespace bar { namespace baz { class qux; } } }",
+  Style);
+
+  // Validate just over the ColumnLimit.
+  verifyFormat("namespace foo {\n"
+   "namespace bar { namespace baz { class quux; } }\n"
+   "}",
+   Style);
+
+  verifyFormat("namespace foo {\n"
+   "namespace bar {\n"
+   "namespace baz { namespace qux { class quux; } }\n"
+   "}\n"
+   "}",
+   Style);
+
+  // Validate that the ColumnLimit logic accounts for trailing content as well.
+  verifyFormat("namespace foo {\n"
+   "namespace bar { namespace baz { class qux; } }\n"
+   "} // extra",
+   Style);
+
+  // No ColumnLimit, allows long nested one-liners, but also leaves multi-line
+  // instances alone.
+  Style.ColumnLimit = 0;
+  verifyFormat(
+  "namespace foo { namespace bar { namespace baz { class qux; } } }",
+  Style);
+
+  verifyNoChange("namespace foo {\n"
+ "namespace bar { namespace baz { class qux; } }\n"
+ "}",
+ Style);

galenelias wrote:

Well, you would probably be more the expert on the intended design of 
'ColumnLimit: 0', but my mental model of ColumnLimit: 0 is basically that it 
will respect the line breaks in the existing code as long as they fall on valid 
'seams'.  This lets you make really long lines, but also lets you produce very 
short lines by the same token, because it's basically taking the existing 
wrapping as the truth, assuming it's a valid wrapping.  I personally really 
value this behavior in the code-base I work on, so I wouldn't consider these 
instances a bug (same goes for the linked discourse link).

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-25 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 1/8] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-25 Thread Galen Elias via cfe-commits



@@ -27981,6 +27981,132 @@ TEST_F(FormatTest, BreakBinaryOperations) {
Style);
 }
 
+TEST_F(FormatTest, ShortNamespacesOption) {
+  auto BaseStyle = getLLVMStyle();
+  BaseStyle.AllowShortNamespacesOnASingleLine = true;
+  BaseStyle.FixNamespaceComments = false;
+
+  auto Style = BaseStyle;
+
+  // Basic functionality.
+  verifyFormat("namespace foo { class bar; }", Style);
+  verifyFormat("namespace foo::bar { class baz; }", Style);
+  verifyFormat("namespace { class bar; }", Style);
+  verifyFormat("namespace foo {\n"
+   "class bar;\n"
+   "class baz;\n"
+   "}",
+   Style);
+
+  // Trailing comments prevent merging.
+  verifyFormat("namespace foo {\n"
+   "namespace baz { class qux; } // comment\n"
+   "}",
+   Style);
+
+  // Make sure code doesn't walk too far on unbalanced code.
+  verifyFormat("namespace foo {", Style);
+  verifyFormat("namespace foo {\n"
+   "class baz;",
+   Style);
+  verifyFormat("namespace foo {\n"
+   "namespace bar { class baz; }",
+   Style);
+
+  // Nested namespaces.
+  verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
+  verifyFormat("namespace foo {\n"
+   "namespace bar { class baz; }\n"
+   "namespace qux { class quux; }\n"
+   "}",
+   Style);
+
+  // Varying inner content.
+  verifyFormat("namespace foo {\n"
+   "int f() { return 5; }\n"
+   "}",
+   Style);
+  verifyFormat("namespace foo { template  struct bar; }", Style);
+  verifyFormat("namespace foo { constexpr int num = 42; }", Style);
+
+  // Validate wrapping scenarios around the ColumnLimit.
+  Style.ColumnLimit = 64;
+
+  // Validate just under the ColumnLimit.
+  verifyFormat(
+  "namespace foo { namespace bar { namespace baz { class qux; } } }",
+  Style);
+
+  // Validate just over the ColumnLimit.
+  verifyFormat("namespace foo {\n"
+   "namespace bar { namespace baz { class quux; } }\n"
+   "}",
+   Style);
+
+  verifyFormat("namespace foo {\n"
+   "namespace bar {\n"
+   "namespace baz { namespace qux { class quux; } }\n"
+   "}\n"
+   "}",
+   Style);
+
+  // Validate that the ColumnLimit logic accounts for trailing content as well.
+  verifyFormat("namespace foo {\n"
+   "namespace bar { namespace baz { class qux; } }\n"
+   "} // extra",
+   Style);
+
+  // No ColumnLimit, allows long nested one-liners, but also leaves multi-line
+  // instances alone.
+  Style.ColumnLimit = 0;
+  verifyFormat(
+  "namespace foo { namespace bar { namespace baz { class qux; } } }",
+  Style);
+
+  verifyNoChange("namespace foo {\n"
+ "namespace bar { namespace baz { class qux; } }\n"
+ "}",
+ Style);
+
+  Style = BaseStyle;
+  Style.CompactNamespaces = true;
+  verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
+
+  // If we can't merge an outer nested namespaces, but can merge an inner
+  // nested namespace, then CompactNamespaces will merge the outer namespace
+  // first, preventing the merging of the inner namespace
+  verifyFormat("namespace foo { namespace baz {\n"
+   "class qux;\n"
+   "} // comment\n"
+   "}",
+   Style);
+
+  // This option doesn't really work with FixNamespaceComments and nested
+  // namespaces. Code should use the concatenated namespace syntax.  e.g.
+  // 'namespace foo::bar'.
+  Style = BaseStyle;
+  Style.FixNamespaceComments = true;
+
+  verifyFormat(
+  "namespace foo {\n"
+  "namespace bar { namespace baz { class qux; } } // namespace bar\n"
+  "} // namespace foo",
+  "namespace foo { namespace bar { namespace baz { class qux; } } }",
+  Style);
+
+  // This option doesn't really make any sense with ShortNamespaceLines = 0.
+  Style.ShortNamespaceLines = 0;
+
+  verifyFormat(
+  "namespace foo {\n"
+  "namespace bar {\n"
+  "namespace baz { class qux; } // namespace baz\n"
+  "} // namespace bar\n"
+  "} // namespace foo",
+  "namespace foo { namespace bar { namespace baz { class qux; } } }",
+  Style);
+}

galenelias wrote:

While I agree on principal, I think that interaction is currently beyond my 
knowledge of how to achieve within the design of clang-format.  The code that 
does the FixNamespaceComments happens before the UnwrappedLineFormatter, so you 
would need to somehow to call 'tryMergeNamespace' from the 
`NamespaceEndCommentsFixer` code to see if the namespace will be merged, then 
avoid adding the comment there.

I think it's OK to leave this interaction sub-optimal for now, with the 
guidance that people should use the 'namespace Foo::Bar' syntax with this 
feature if they wan

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-09-26 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 1/9] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-10-15 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 01/10] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 2af1d4065c3cc1..c96fc4d1d9557b 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -972,6 +972,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 97fac41cdd3008..495b727e609df6 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -952,6 +952,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1457,6 +1459,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] clang-format: Add "AllowShortNamespacesOnASingleLine" option (PR #105597)

2024-10-15 Thread Galen Elias via cfe-commits

galenelias wrote:

> I'm inclined to think that all the AllowShort options should work the same 
> way whether ColumnLimit is 0. However, a couple of them I checked 
> (AllowShortLoops and AllowShortBlocks) have no effect unless set to false. I 
> suggest that we mark the test as a FIXME or don't include it here.

As an active user of ColumnLimit 0, I disagree.  At least for my team we use 
ColumnLimit of 0 to allow for more customization and variability of the column 
limit.  Treating it like ColumnLimit: Infinity causes huge constructs to be 
pulled into a single line for some of these 'AllowShort' options, which we 
actively don't want.  Making somewhat arbitrary and contextual decisions about 
how to do the breaking is a 'feature' in my opinion - although I'm not sure if 
that's truly the intent and vision behind ColumnLimit 0.

My thought here is that this feature should function similarly to how 
CompactNamespaces interacts with ColumnLimit: 0, which is basically that it 
allows arbitrary line breaking.  So you could have a 1000 character line, or 
you could break it after every namespace.  Forcing it to always pull single 
statements no matter how long onto a single line would be undesirable in my 
opinion.

I'm definitely open to changing things here, and being consistent if there is 
some other vision of how features are supposed to interact with ColumnLimit 0, 
but this has been my mental model so far, and that seems to be the natural 
behavior that I've observed, so validating that functionality in the tests 
seems reasonable to me.

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Add `AllowShortNamespacesOnASingleLine` option (PR #105597)

2024-12-29 Thread Galen Elias via cfe-commits



@@ -4504,6 +4504,16 @@ TEST_F(FormatTest, FormatsCompactNamespaces) {
"} // namespace bb\n"
"} // namespace aa",
Style);
+
+  verifyFormat("namespace a { namespace b { namespace c {\n"
+   "}}} // namespace a::b::c",
+   Style);
+
+  verifyFormat("namespace a { namespace b {\n"
+   "namespace cc {\n"
+   "}}} // namespace a::b::cc",

galenelias wrote:

I'll defer to your judgement, but I personally think it is helpful to test each 
side of these edge cases around line wrapping.  Having a test to verify things 
don't get wrapped at 'X' columns, but then having tests cases which are like 
X/2 long doesn't really validate the precision of the width calculations, as 
evidenced by this bug, and means we might be wrapping too aggressively, and 
won't catch that via any existing tests.  If we only verify lines significantly 
shorter or longer than the ColumnLimit then we it is easy for mistakes in the 
logic to sneak through.

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Add `AllowShortNamespacesOnASingleLine` option (PR #105597)

2024-12-28 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 93eb3d89652607173f4f68fce7dcc5b2bd33f266 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 01/15] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 6383934afa2c40..26cd673685942e 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -988,6 +988,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 95129a8fe9240c..8f44e9f00212cf 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -975,6 +975,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1480,6 +1482,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] [clang-format] Add `AllowShortNamespacesOnASingleLine` option (PR #105597)

2024-12-29 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 93eb3d89652607173f4f68fce7dcc5b2bd33f266 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 01/16] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 6383934afa2c40..26cd673685942e 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -988,6 +988,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 95129a8fe9240c..8f44e9f00212cf 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -975,6 +975,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1480,6 +1482,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] [clang-format] Add `AllowShortNamespacesOnASingleLine` option (PR #105597)

2024-12-27 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/105597

>From 93eb3d89652607173f4f68fce7dcc5b2bd33f266 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 01/14] clang-format: Add "AllowShortNamespacesOnASingleLine"
 option

This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851

The feature is to allow code like the following not to be broken across
multiple lines:

```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```

Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
 clang/include/clang/Format/Format.h |   5 +
 clang/lib/Format/Format.cpp |   3 +
 clang/lib/Format/UnwrappedLineFormatter.cpp |  82 ++
 clang/unittests/Format/FormatTest.cpp   | 112 
 4 files changed, 202 insertions(+)

diff --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index 6383934afa2c40..26cd673685942e 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -988,6 +988,11 @@ struct FormatStyle {
   /// \version 3.7
   bool AllowShortLoopsOnASingleLine;
 
+  /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+  /// line.
+  /// \version 19
+  bool AllowShortNamespacesOnASingleLine;
+
   /// Different ways to break after the function definition return type.
   /// This option is **deprecated** and is retained for backwards 
compatibility.
   enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 95129a8fe9240c..8f44e9f00212cf 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -975,6 +975,8 @@ template <> struct MappingTraits {
Style.AllowShortLambdasOnASingleLine);
 IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+IO.mapOptional("AllowShortNamespacesOnASingleLine",
+   Style.AllowShortNamespacesOnASingleLine);
 IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
 IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1480,6 +1482,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind 
Language) {
   LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
   LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
   LLVMStyle.AllowShortLoopsOnASingleLine = false;
+  LLVMStyle.AllowShortNamespacesOnASingleLine = false;
   LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
 TheLine->First != LastNonComment) {
   return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
 }
+
+if (TheLine->Last->is(tok::l_brace)) {
+  if (Style.AllowShortNamespacesOnASingleLine &&
+  TheLine->First->is(tok::kw_namespace)) {
+if (unsigned result = tryMergeNamespace(I, E, Limit))
+  return result;
+  }
+}
+
 // Try to merge a control statement block with left brace unwrapped.
 if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
 FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if so
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(TT_LineComment))
+return 0;
+
+  unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+  unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+  if (!inner_result)
+

[clang] [clang-format] Add `AllowShortNamespacesOnASingleLine` option (PR #105597)

2024-12-27 Thread Galen Elias via cfe-commits



@@ -616,6 +627,63 @@ class LineJoiner {
 return 1;
   }
 
+  unsigned tryMergeNamespace(SmallVectorImpl::const_iterator 
I,
+ SmallVectorImpl::const_iterator 
E,
+ unsigned Limit) {
+if (Limit == 0)
+  return 0;
+if (I[1]->InPPDirective != (*I)->InPPDirective ||
+(I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+  return 0;
+}
+if (I + 2 == E || I[2]->Type == LT_Invalid)
+  return 0;
+
+Limit = limitConsideringMacros(I + 1, E, Limit);
+
+if (!nextTwoLinesFitInto(I, Limit))
+  return 0;
+
+// Check if it's a namespace inside a namespace, and call recursively if 
so.
+// '3' is the sizes of the whitespace and closing brace for " _inner_ }".
+if (I[1]->First->is(tok::kw_namespace)) {
+  if (I[1]->Last->is(tok::comment) || !Style.CompactNamespaces)
+return 0;
+
+  assert(Limit >= I[1]->Last->TotalLength + 3);
+  const unsigned InnerLimit = Limit - I[1]->Last->TotalLength - 3;
+  const unsigned MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+  if (!MergedLines)

galenelias wrote:

Sorry, missed the `if (MergedLines == 0)` suggestion.  The diffing UI on GitHub 
PRs is surprisingly blunt.

https://github.com/llvm/llvm-project/pull/105597
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)

2025-02-11 Thread Galen Elias via cfe-commits



@@ -28430,6 +28430,36 @@ TEST_F(FormatTest, ShortNamespacesOption) {
   "}}} // namespace foo::bar::baz",
   "namespace foo { namespace bar { namespace baz { class qux; } } }",
   Style);
+  Style.FixNamespaceComments = false;
+
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterNamespace = true;
+  verifyFormat("namespace foo { class bar; }", Style);
+  verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
+  verifyFormat("namespace foo\n"
+   "{ // comment\n"

galenelias wrote:

Interesting.  This seems to be quirk with the existing 
`BraceWrapping.AfterNamespace` behavior, but nothing specific to this PR.

https://github.com/llvm/llvm-project/pull/123010
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)

2025-01-22 Thread Galen Elias via cfe-commits

galenelias wrote:

> Is there an github "Issue" for this? I can't quite understand what problem we 
> are trying to fix.

There is not a GitHub Issue filed yet, but I could get one filed if that would 
be helpful.

The issue is just there are users who want to use 
`AllowShortNamespacesOnASingleLine=true`, but who also currently use 
`BraceWrapping.AfterNamespace=true`.

So, they want the following code:

```cpp
namespace Foo
{
struct Bar;
}
```

To be formatted as:
```cpp
namespace Foo { struct Bar; }
```

But currently that isn't working.  AllowShortNamespacesOnASingleLine is 
currently written assuming BraceWrapping.AfterNamespace == false (which is the 
more common style).

https://github.com/llvm/llvm-project/pull/123010
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)

2025-01-21 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/123010

>From 9d60d4980f1edbdd4cd4a9499f69e9d225717238 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Tue, 14 Jan 2025 20:44:10 -0800
Subject: [PATCH 1/2] Support BraceWrapping.AfterNamespace with
 AllowShortNamespacesOnASingleLine

AllowShortNamespacesOnASingleLine assumes that there is no newline
before the namespace brace, however, there is no actual reason this
shouldn't be compatible with BraceWrapping.AfterNamespace = true.

This is a little tricky in the implementation because
UnwrappedLineFormatter works on lines, so being flexible about the
offsets is awkard.

Not sure if there is a better pattern for combining the 'AllowShort'
options with the various configurations of BraceWrapping, but this
seemed mostly reasonable.  Really, it would almost be preferable to just
pattern match on the direct token stream, rathern than the
AnnotatedLines, but I'm not seeing a straightforward way to do that.
---
 clang/lib/Format/UnwrappedLineFormatter.cpp | 45 +
 clang/unittests/Format/FormatTest.cpp   | 20 +
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index cee84fb1191abb..787136a26b378e 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -367,8 +367,12 @@ class LineJoiner {
 
 if (Style.AllowShortNamespacesOnASingleLine &&
 TheLine->First->is(tok::kw_namespace) &&
-TheLine->Last->is(tok::l_brace)) {
-  const auto result = tryMergeNamespace(I, E, Limit);
+((Style.BraceWrapping.AfterNamespace &&
+  NextLine.First->is(tok::l_brace)) ||
+ (!Style.BraceWrapping.AfterNamespace &&
+  TheLine->Last->is(tok::l_brace {
+  const auto result =
+  tryMergeNamespace(I, E, Limit, Style.BraceWrapping.AfterNamespace);
   if (result > 0)
 return result;
 }
@@ -628,28 +632,36 @@ class LineJoiner {
 
   unsigned tryMergeNamespace(ArrayRef::const_iterator I,
  ArrayRef::const_iterator E,
- unsigned Limit) {
+ unsigned Limit, bool OpenBraceWrapped) {
 if (Limit == 0)
   return 0;
 
-assert(I[1]);
-const auto &L1 = *I[1];
+// The merging code is relative to the opening namespace brace, which could
+// be either on the first or second line due to the brace wrapping rules.
+const size_t OpeningBraceLineOffset = OpenBraceWrapped ? 1 : 0;
+const auto BraceOpenLine = I + OpeningBraceLineOffset;
+
+if (std::distance(BraceOpenLine, E) <= 2)
+  return 0;
+
+if (BraceOpenLine[0]->Last->is(TT_LineComment))
+  return 0;
+
+assert(BraceOpenLine[1]);
+const auto &L1 = *BraceOpenLine[1];
 if (L1.InPPDirective != (*I)->InPPDirective ||
 (L1.InPPDirective && L1.First->HasUnescapedNewline)) {
   return 0;
 }
 
-if (std::distance(I, E) <= 2)
-  return 0;
-
-assert(I[2]);
-const auto &L2 = *I[2];
+assert(BraceOpenLine[2]);
+const auto &L2 = *BraceOpenLine[2];
 if (L2.Type == LT_Invalid)
   return 0;
 
 Limit = limitConsideringMacros(I + 1, E, Limit);
 
-if (!nextTwoLinesFitInto(I, Limit))
+if (!nextNLinesFitInto(I, I + OpeningBraceLineOffset + 2, Limit))
   return 0;
 
 // Check if it's a namespace inside a namespace, and call recursively if 
so.
@@ -660,17 +672,18 @@ class LineJoiner {
 
   assert(Limit >= L1.Last->TotalLength + 3);
   const auto InnerLimit = Limit - L1.Last->TotalLength - 3;
-  const auto MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+  const auto MergedLines =
+  tryMergeNamespace(BraceOpenLine + 1, E, InnerLimit, 
OpenBraceWrapped);
   if (MergedLines == 0)
 return 0;
-  const auto N = MergedLines + 2;
+  const auto N = MergedLines + 2 + OpeningBraceLineOffset;
   // Check if there is even a line after the inner result.
   if (std::distance(I, E) <= N)
 return 0;
   // Check that the line after the inner result starts with a closing brace
   // which we are permitted to merge into one line.
   if (I[N]->First->is(tok::r_brace) && !I[N]->First->MustBreakBefore &&
-  I[MergedLines + 1]->Last->isNot(tok::comment) &&
+  BraceOpenLine[MergedLines + 1]->Last->isNot(tok::comment) &&
   nextNLinesFitInto(I, I + N + 1, Limit)) {
 return N;
   }
@@ -688,8 +701,8 @@ class LineJoiner {
 if (L2.First->isNot(tok::r_brace) || L2.First->MustBreakBefore)
   return 0;
 
-// If so, merge all three lines.
-return 2;
+// If so, merge all lines.
+return 2 + OpeningBraceLineOffset;
   }
 
   unsigned
diff --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index 4d48bcacddead8..bf008e61490f57 100644
--- a/

[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)

2025-01-27 Thread Galen Elias via cfe-commits



@@ -628,28 +632,36 @@ class LineJoiner {
 
   unsigned tryMergeNamespace(ArrayRef::const_iterator I,
  ArrayRef::const_iterator E,
- unsigned Limit) {
+ unsigned Limit, bool OpenBraceWrapped) {
 if (Limit == 0)
   return 0;
 
-assert(I[1]);
-const auto &L1 = *I[1];
+// The merging code is relative to the opening namespace brace, which could
+// be either on the first or second line due to the brace wrapping rules.
+const size_t OpeningBraceLineOffset = OpenBraceWrapped ? 1 : 0;

galenelias wrote:

Oops, apologies for missing the warning.

https://github.com/llvm/llvm-project/pull/123010
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)

2025-01-27 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/123010

>From 9d60d4980f1edbdd4cd4a9499f69e9d225717238 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Tue, 14 Jan 2025 20:44:10 -0800
Subject: [PATCH 1/3] Support BraceWrapping.AfterNamespace with
 AllowShortNamespacesOnASingleLine

AllowShortNamespacesOnASingleLine assumes that there is no newline
before the namespace brace, however, there is no actual reason this
shouldn't be compatible with BraceWrapping.AfterNamespace = true.

This is a little tricky in the implementation because
UnwrappedLineFormatter works on lines, so being flexible about the
offsets is awkard.

Not sure if there is a better pattern for combining the 'AllowShort'
options with the various configurations of BraceWrapping, but this
seemed mostly reasonable.  Really, it would almost be preferable to just
pattern match on the direct token stream, rathern than the
AnnotatedLines, but I'm not seeing a straightforward way to do that.
---
 clang/lib/Format/UnwrappedLineFormatter.cpp | 45 +
 clang/unittests/Format/FormatTest.cpp   | 20 +
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index cee84fb1191abb..787136a26b378e 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -367,8 +367,12 @@ class LineJoiner {
 
 if (Style.AllowShortNamespacesOnASingleLine &&
 TheLine->First->is(tok::kw_namespace) &&
-TheLine->Last->is(tok::l_brace)) {
-  const auto result = tryMergeNamespace(I, E, Limit);
+((Style.BraceWrapping.AfterNamespace &&
+  NextLine.First->is(tok::l_brace)) ||
+ (!Style.BraceWrapping.AfterNamespace &&
+  TheLine->Last->is(tok::l_brace {
+  const auto result =
+  tryMergeNamespace(I, E, Limit, Style.BraceWrapping.AfterNamespace);
   if (result > 0)
 return result;
 }
@@ -628,28 +632,36 @@ class LineJoiner {
 
   unsigned tryMergeNamespace(ArrayRef::const_iterator I,
  ArrayRef::const_iterator E,
- unsigned Limit) {
+ unsigned Limit, bool OpenBraceWrapped) {
 if (Limit == 0)
   return 0;
 
-assert(I[1]);
-const auto &L1 = *I[1];
+// The merging code is relative to the opening namespace brace, which could
+// be either on the first or second line due to the brace wrapping rules.
+const size_t OpeningBraceLineOffset = OpenBraceWrapped ? 1 : 0;
+const auto BraceOpenLine = I + OpeningBraceLineOffset;
+
+if (std::distance(BraceOpenLine, E) <= 2)
+  return 0;
+
+if (BraceOpenLine[0]->Last->is(TT_LineComment))
+  return 0;
+
+assert(BraceOpenLine[1]);
+const auto &L1 = *BraceOpenLine[1];
 if (L1.InPPDirective != (*I)->InPPDirective ||
 (L1.InPPDirective && L1.First->HasUnescapedNewline)) {
   return 0;
 }
 
-if (std::distance(I, E) <= 2)
-  return 0;
-
-assert(I[2]);
-const auto &L2 = *I[2];
+assert(BraceOpenLine[2]);
+const auto &L2 = *BraceOpenLine[2];
 if (L2.Type == LT_Invalid)
   return 0;
 
 Limit = limitConsideringMacros(I + 1, E, Limit);
 
-if (!nextTwoLinesFitInto(I, Limit))
+if (!nextNLinesFitInto(I, I + OpeningBraceLineOffset + 2, Limit))
   return 0;
 
 // Check if it's a namespace inside a namespace, and call recursively if 
so.
@@ -660,17 +672,18 @@ class LineJoiner {
 
   assert(Limit >= L1.Last->TotalLength + 3);
   const auto InnerLimit = Limit - L1.Last->TotalLength - 3;
-  const auto MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+  const auto MergedLines =
+  tryMergeNamespace(BraceOpenLine + 1, E, InnerLimit, 
OpenBraceWrapped);
   if (MergedLines == 0)
 return 0;
-  const auto N = MergedLines + 2;
+  const auto N = MergedLines + 2 + OpeningBraceLineOffset;
   // Check if there is even a line after the inner result.
   if (std::distance(I, E) <= N)
 return 0;
   // Check that the line after the inner result starts with a closing brace
   // which we are permitted to merge into one line.
   if (I[N]->First->is(tok::r_brace) && !I[N]->First->MustBreakBefore &&
-  I[MergedLines + 1]->Last->isNot(tok::comment) &&
+  BraceOpenLine[MergedLines + 1]->Last->isNot(tok::comment) &&
   nextNLinesFitInto(I, I + N + 1, Limit)) {
 return N;
   }
@@ -688,8 +701,8 @@ class LineJoiner {
 if (L2.First->isNot(tok::r_brace) || L2.First->MustBreakBefore)
   return 0;
 
-// If so, merge all three lines.
-return 2;
+// If so, merge all lines.
+return 2 + OpeningBraceLineOffset;
   }
 
   unsigned
diff --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index 4d48bcacddead8..bf008e61490f57 100644
--- a/

[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)

2025-01-14 Thread Galen Elias via cfe-commits


https://github.com/galenelias created 
https://github.com/llvm/llvm-project/pull/123010

AllowShortNamespacesOnASingleLine assumes that there is no newline before the 
namespace brace, however, there is no actual reason this shouldn't be 
compatible with BraceWrapping.AfterNamespace = true.

This is a little tricky in the implementation because UnwrappedLineFormatter 
works on lines, so being flexible about the offsets is awkward.

Not sure if there is a better pattern for combining the 'AllowShort' options 
with the various configurations of BraceWrapping, but this seemed mostly 
reasonable.  Really, it would almost be preferable to just pattern match on the 
direct token stream, rather than the AnnotatedLines, but I'm not seeing a 
straightforward way to do that.

>From 9d60d4980f1edbdd4cd4a9499f69e9d225717238 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Tue, 14 Jan 2025 20:44:10 -0800
Subject: [PATCH] Support BraceWrapping.AfterNamespace with
 AllowShortNamespacesOnASingleLine

AllowShortNamespacesOnASingleLine assumes that there is no newline
before the namespace brace, however, there is no actual reason this
shouldn't be compatible with BraceWrapping.AfterNamespace = true.

This is a little tricky in the implementation because
UnwrappedLineFormatter works on lines, so being flexible about the
offsets is awkard.

Not sure if there is a better pattern for combining the 'AllowShort'
options with the various configurations of BraceWrapping, but this
seemed mostly reasonable.  Really, it would almost be preferable to just
pattern match on the direct token stream, rathern than the
AnnotatedLines, but I'm not seeing a straightforward way to do that.
---
 clang/lib/Format/UnwrappedLineFormatter.cpp | 45 +
 clang/unittests/Format/FormatTest.cpp   | 20 +
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index cee84fb1191abb..787136a26b378e 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -367,8 +367,12 @@ class LineJoiner {
 
 if (Style.AllowShortNamespacesOnASingleLine &&
 TheLine->First->is(tok::kw_namespace) &&
-TheLine->Last->is(tok::l_brace)) {
-  const auto result = tryMergeNamespace(I, E, Limit);
+((Style.BraceWrapping.AfterNamespace &&
+  NextLine.First->is(tok::l_brace)) ||
+ (!Style.BraceWrapping.AfterNamespace &&
+  TheLine->Last->is(tok::l_brace {
+  const auto result =
+  tryMergeNamespace(I, E, Limit, Style.BraceWrapping.AfterNamespace);
   if (result > 0)
 return result;
 }
@@ -628,28 +632,36 @@ class LineJoiner {
 
   unsigned tryMergeNamespace(ArrayRef::const_iterator I,
  ArrayRef::const_iterator E,
- unsigned Limit) {
+ unsigned Limit, bool OpenBraceWrapped) {
 if (Limit == 0)
   return 0;
 
-assert(I[1]);
-const auto &L1 = *I[1];
+// The merging code is relative to the opening namespace brace, which could
+// be either on the first or second line due to the brace wrapping rules.
+const size_t OpeningBraceLineOffset = OpenBraceWrapped ? 1 : 0;
+const auto BraceOpenLine = I + OpeningBraceLineOffset;
+
+if (std::distance(BraceOpenLine, E) <= 2)
+  return 0;
+
+if (BraceOpenLine[0]->Last->is(TT_LineComment))
+  return 0;
+
+assert(BraceOpenLine[1]);
+const auto &L1 = *BraceOpenLine[1];
 if (L1.InPPDirective != (*I)->InPPDirective ||
 (L1.InPPDirective && L1.First->HasUnescapedNewline)) {
   return 0;
 }
 
-if (std::distance(I, E) <= 2)
-  return 0;
-
-assert(I[2]);
-const auto &L2 = *I[2];
+assert(BraceOpenLine[2]);
+const auto &L2 = *BraceOpenLine[2];
 if (L2.Type == LT_Invalid)
   return 0;
 
 Limit = limitConsideringMacros(I + 1, E, Limit);
 
-if (!nextTwoLinesFitInto(I, Limit))
+if (!nextNLinesFitInto(I, I + OpeningBraceLineOffset + 2, Limit))
   return 0;
 
 // Check if it's a namespace inside a namespace, and call recursively if 
so.
@@ -660,17 +672,18 @@ class LineJoiner {
 
   assert(Limit >= L1.Last->TotalLength + 3);
   const auto InnerLimit = Limit - L1.Last->TotalLength - 3;
-  const auto MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+  const auto MergedLines =
+  tryMergeNamespace(BraceOpenLine + 1, E, InnerLimit, 
OpenBraceWrapped);
   if (MergedLines == 0)
 return 0;
-  const auto N = MergedLines + 2;
+  const auto N = MergedLines + 2 + OpeningBraceLineOffset;
   // Check if there is even a line after the inner result.
   if (std::distance(I, E) <= N)
 return 0;
   // Check that the line after the inner result starts with a closing brace
   // which we are permitted to merge into one line.
   if (I[N]->Fi

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-04-17 Thread Galen Elias via cfe-commits


https://github.com/galenelias created 
https://github.com/llvm/llvm-project/pull/136167

This addresses:

Issue #77341: clang-tidy fails to parse config files silently, even with 
--verify-config.

Currently when clang-tidy attempts to parse a .clang-tidy config file with 
invalid syntax, it emits and error and moves on, which can often result in 
applying the default set of checks, which is undesirable.

This change bubbles up the error state from parsing the configuration files up 
to `clangTidyMain` so we can do an early return with an error code. It's 
unfortunately quite a bit of plumbing - but this seemed to be the idiomatic 
approach for the codebase.

>From f54d980bafd76863ab63e68f0568dcfc3329fd9f Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 16 Apr 2025 12:48:18 -0700
Subject: [PATCH] [clang-tidy] Return error code on config parse error

This addresses:

Issue 77341: clang-tidy fails to parse config files silently, even with
--verify-config.

Currently when clang-tidy attempts to parse a .clang-tidy config file
with invalid syntax, it emits and error and moves on, which can often
result in applying the default set of checks, which is undesirable.

This change bubbles up the error state from parsing the configuration
files up to `clangTidyMain` so we can do an early return with an error
code. It's unfortunately quite a bit of plumbing - but this seemed to be
the idiomatic approach for the clang-tidy codebase.
---
 .../ClangTidyDiagnosticConsumer.cpp   |  2 +-
 .../clang-tidy/ClangTidyOptions.cpp   | 66 +++
 .../clang-tidy/ClangTidyOptions.h | 23 ---
 .../clang-tidy/tool/ClangTidyMain.cpp | 47 +++--
 .../clang-tidy/OptionsProviderTest.cpp| 16 ++---
 5 files changed, 91 insertions(+), 63 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 731141a545a48..4691b1606986b 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -266,7 +266,7 @@ ClangTidyOptions 
ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
   return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  *OptionsProvider->getOptions(File), 0);
 }
 
 void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index dd1d86882f5d4..7ae23c137106e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -267,16 +267,19 @@ const char
 ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] =
 "command-line option '-config'";
 
-ClangTidyOptions
+llvm::ErrorOr
 ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) {
   ClangTidyOptions Result;
   unsigned Priority = 0;
-  for (auto &Source : getRawOptions(FileName))
+  llvm::ErrorOr> Options = getRawOptions(FileName);
+  if (!Options)
+return Options.getError();
+  for (auto &Source : *Options)
 Result.mergeWith(Source.first, ++Priority);
   return Result;
 }
 
-std::vector
+llvm::ErrorOr>
 DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector Result;
   Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
@@ -292,10 +295,12 @@ ConfigOptionsProvider::ConfigOptionsProvider(
   std::move(OverrideOptions), std::move(FS)),
   ConfigOptions(std::move(ConfigOptions)) {}
 
-std::vector
+llvm::ErrorOr>
 ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
-  std::vector RawOptions =
+  llvm::ErrorOr> RawOptions =
   DefaultOptionsProvider::getRawOptions(FileName);
+  if (!RawOptions)
+return RawOptions;
   if (ConfigOptions.InheritParentConfig.value_or(false)) {
 LLVM_DEBUG(llvm::dbgs()
<< "Getting options for file " << FileName << "...\n");
@@ -303,13 +308,13 @@ ConfigOptionsProvider::getRawOptions(llvm::StringRef 
FileName) {
 llvm::ErrorOr> AbsoluteFilePath =
 getNormalizedAbsolutePath(FileName);
 if (AbsoluteFilePath) {
-  addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
+  addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
 }
   }
-  RawOptions.emplace_back(ConfigOptions,
-  OptionsSourceTypeConfigCommandLineOption);
-  RawOptions.emplace_back(OverrideOptions,
-  OptionsSourceTypeCheckCommandLineOption);
+  RawOptions->emplace_back(ConfigOptions,
+   OptionsSourceTypeConfigCommandLineOption);
+  RawOptions->emplace_back(OverrideOptions,
+   OptionsSourceTypeCheckCommandLineOption);
   return RawOptions;
 }

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-04-30 Thread Galen Elias via cfe-commits



@@ -337,8 +337,7 @@ Allow empty enabled checks. This suppresses
 the "no checks enabled" error when disabling
 all of the checks.
 )"),
- cl::init(false),
- cl::cat(ClangTidyCategory));
+   cl::init(false), 
cl::cat(ClangTidyCategory));

galenelias wrote:

Oops, sorry.  It would be nice if the files were 'clang-format clean' as I am 
used to a flow where I can just run `clang-format -i` on all my modified files 
to ensure I'm following the formatting guidelines.  That is unfortunately not 
the case here.

https://github.com/llvm/llvm-project/pull/136167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-04-30 Thread Galen Elias via cfe-commits



@@ -46,20 +46,20 @@ TEST(ClangTidyOptionsProvider, InMemoryFileSystems) {
 
   FileOptionsProvider FileOpt({}, {}, {}, FileSystem);
 
-  ClangTidyOptions File1Options =
+  llvm::ErrorOr File1Options =
   FileOpt.getOptions("ProjectRoot/SubDir1/File.cpp");
-  ClangTidyOptions File2Options =
+  llvm::ErrorOr File2Options =
   FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/File.cpp");
-  ClangTidyOptions File3Options =
+  llvm::ErrorOr File3Options =
   FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/SubDir3/File.cpp");
 
-  ASSERT_TRUE(File1Options.Checks.has_value());
-  EXPECT_EQ(*File1Options.Checks, "-*,clang-diagnostic-*,readability-*");
-  ASSERT_TRUE(File2Options.Checks.has_value());
-  EXPECT_EQ(*File2Options.Checks, "bugprone-*,misc-*,clang-diagnostic-*");
+  ASSERT_TRUE(File1Options->Checks.has_value());
+  EXPECT_EQ(*File1Options->Checks, "-*,clang-diagnostic-*,readability-*");
+  ASSERT_TRUE(File2Options->Checks.has_value());
+  EXPECT_EQ(*File2Options->Checks, "bugprone-*,misc-*,clang-diagnostic-*");
 
   // 2 and 3 should use the same config so these should also be the same.
-  EXPECT_EQ(File2Options.Checks, File3Options.Checks);
+  EXPECT_EQ(File2Options->Checks, File3Options->Checks);
 }

galenelias wrote:

I couldn't find any tests which exercise `clangTidyMain` (which makes sense, as 
that is not really the layer a unit test would operate on.  The best place I 
could find to put a test was near the 
ClangTidyOptionsProvider.InMemoryFileSystems test, which at least tests the 
behavior with multiple configuration files, as well as some amount of the 
propagation logic.

I will go ahead and add a test there unless there is a more appropriate spot 
for verifying the high level "early exit code 1" behavior of the PR.

https://github.com/llvm/llvm-project/pull/136167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-01 Thread Galen Elias via cfe-commits



@@ -266,7 +266,7 @@ ClangTidyOptions 
ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
   return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  *OptionsProvider->getOptions(File), 0);

galenelias wrote:

Ok, so my current plan is:
* Loop through all input files in clangTidyMain to explicitly validate configs 
for each input file and be able to return an error code there if a failure is 
found.
* In this method (`ClangTidyContext::getOptionsForFile`) just return the 
default config options if an error is found, which is similar to the behavior 
before this PR.  It seems like trying to make getOptionsForFile return an 
ErrorOr object and handle that at each call site is very cumbersome and leads 
to a lot of non-obvious failure modes that need to be resolved.

https://github.com/llvm/llvm-project/pull/136167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-01 Thread Galen Elias via cfe-commits



@@ -266,7 +266,7 @@ ClangTidyOptions 
ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
   return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  *OptionsProvider->getOptions(File), 0);

galenelias wrote:

My analysis was that because clangTidyMain does the validation explicitly up 
front and returns an error if it can't parse the config, that once it runs 
through here, we are basically guaranteed that it will succeed, because the 
errors have been caught.

However, digging in a bit deeper, I think there are at least two holes in this 
understanding: clangTidyMain only pre-validates the config for the first file 
passed in, so passing in a second file in a different directory with an invalid 
configuration will crash here.  The second is potentially the 'ClangTidyPlugin' 
code, which seems to be another entry point into the clang tidy code that 
bypasses clangTidyMain.  I'm not sure what the ClangTidyPlugin is or how to 
validate its behavior in the face of an invalid config.

I'll need to do a bit more digging to figure out the right strategy here.  Just 
bubbling up ErrorOr returns doesn't always seem great, but might unfortunately 
be required.

https://github.com/llvm/llvm-project/pull/136167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-01 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/136167

>From f54d980bafd76863ab63e68f0568dcfc3329fd9f Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 16 Apr 2025 12:48:18 -0700
Subject: [PATCH 1/6] [clang-tidy] Return error code on config parse error

This addresses:

Issue 77341: clang-tidy fails to parse config files silently, even with
--verify-config.

Currently when clang-tidy attempts to parse a .clang-tidy config file
with invalid syntax, it emits and error and moves on, which can often
result in applying the default set of checks, which is undesirable.

This change bubbles up the error state from parsing the configuration
files up to `clangTidyMain` so we can do an early return with an error
code. It's unfortunately quite a bit of plumbing - but this seemed to be
the idiomatic approach for the clang-tidy codebase.
---
 .../ClangTidyDiagnosticConsumer.cpp   |  2 +-
 .../clang-tidy/ClangTidyOptions.cpp   | 66 +++
 .../clang-tidy/ClangTidyOptions.h | 23 ---
 .../clang-tidy/tool/ClangTidyMain.cpp | 47 +++--
 .../clang-tidy/OptionsProviderTest.cpp| 16 ++---
 5 files changed, 91 insertions(+), 63 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 731141a545a48..4691b1606986b 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -266,7 +266,7 @@ ClangTidyOptions 
ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
   return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  *OptionsProvider->getOptions(File), 0);
 }
 
 void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index dd1d86882f5d4..7ae23c137106e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -267,16 +267,19 @@ const char
 ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] =
 "command-line option '-config'";
 
-ClangTidyOptions
+llvm::ErrorOr
 ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) {
   ClangTidyOptions Result;
   unsigned Priority = 0;
-  for (auto &Source : getRawOptions(FileName))
+  llvm::ErrorOr> Options = getRawOptions(FileName);
+  if (!Options)
+return Options.getError();
+  for (auto &Source : *Options)
 Result.mergeWith(Source.first, ++Priority);
   return Result;
 }
 
-std::vector
+llvm::ErrorOr>
 DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector Result;
   Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
@@ -292,10 +295,12 @@ ConfigOptionsProvider::ConfigOptionsProvider(
   std::move(OverrideOptions), std::move(FS)),
   ConfigOptions(std::move(ConfigOptions)) {}
 
-std::vector
+llvm::ErrorOr>
 ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
-  std::vector RawOptions =
+  llvm::ErrorOr> RawOptions =
   DefaultOptionsProvider::getRawOptions(FileName);
+  if (!RawOptions)
+return RawOptions;
   if (ConfigOptions.InheritParentConfig.value_or(false)) {
 LLVM_DEBUG(llvm::dbgs()
<< "Getting options for file " << FileName << "...\n");
@@ -303,13 +308,13 @@ ConfigOptionsProvider::getRawOptions(llvm::StringRef 
FileName) {
 llvm::ErrorOr> AbsoluteFilePath =
 getNormalizedAbsolutePath(FileName);
 if (AbsoluteFilePath) {
-  addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
+  addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
 }
   }
-  RawOptions.emplace_back(ConfigOptions,
-  OptionsSourceTypeConfigCommandLineOption);
-  RawOptions.emplace_back(OverrideOptions,
-  OptionsSourceTypeCheckCommandLineOption);
+  RawOptions->emplace_back(ConfigOptions,
+   OptionsSourceTypeConfigCommandLineOption);
+  RawOptions->emplace_back(OverrideOptions,
+   OptionsSourceTypeCheckCommandLineOption);
   return RawOptions;
 }
 
@@ -345,21 +350,21 @@ 
FileOptionsBaseProvider::getNormalizedAbsolutePath(llvm::StringRef Path) {
   return NormalizedAbsolutePath;
 }
 
-void FileOptionsBaseProvider::addRawFileOptions(
+std::error_code FileOptionsBaseProvider::addRawFileOptions(
 llvm::StringRef AbsolutePath, std::vector &CurOptions) {
   auto CurSize = CurOptions.size();
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
   auto Memori

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-01 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/136167

>From b8faa7bb02fb4aa0504978dad1a28493fc745786 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 16 Apr 2025 12:48:18 -0700
Subject: [PATCH 1/6] [clang-tidy] Return error code on config parse error

This addresses:

Issue 77341: clang-tidy fails to parse config files silently, even with
--verify-config.

Currently when clang-tidy attempts to parse a .clang-tidy config file
with invalid syntax, it emits and error and moves on, which can often
result in applying the default set of checks, which is undesirable.

This change bubbles up the error state from parsing the configuration
files up to `clangTidyMain` so we can do an early return with an error
code. It's unfortunately quite a bit of plumbing - but this seemed to be
the idiomatic approach for the clang-tidy codebase.
---
 .../ClangTidyDiagnosticConsumer.cpp   |  2 +-
 .../clang-tidy/ClangTidyOptions.cpp   | 66 +++
 .../clang-tidy/ClangTidyOptions.h | 23 ---
 .../clang-tidy/tool/ClangTidyMain.cpp | 47 +++--
 .../clang-tidy/OptionsProviderTest.cpp| 16 ++---
 5 files changed, 91 insertions(+), 63 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index b216970bfbd8c..0413c39bc58eb 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -266,7 +266,7 @@ ClangTidyOptions 
ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
   return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  *OptionsProvider->getOptions(File), 0);
 }
 
 void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index dd1d86882f5d4..7ae23c137106e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -267,16 +267,19 @@ const char
 ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] =
 "command-line option '-config'";
 
-ClangTidyOptions
+llvm::ErrorOr
 ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) {
   ClangTidyOptions Result;
   unsigned Priority = 0;
-  for (auto &Source : getRawOptions(FileName))
+  llvm::ErrorOr> Options = getRawOptions(FileName);
+  if (!Options)
+return Options.getError();
+  for (auto &Source : *Options)
 Result.mergeWith(Source.first, ++Priority);
   return Result;
 }
 
-std::vector
+llvm::ErrorOr>
 DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector Result;
   Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
@@ -292,10 +295,12 @@ ConfigOptionsProvider::ConfigOptionsProvider(
   std::move(OverrideOptions), std::move(FS)),
   ConfigOptions(std::move(ConfigOptions)) {}
 
-std::vector
+llvm::ErrorOr>
 ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
-  std::vector RawOptions =
+  llvm::ErrorOr> RawOptions =
   DefaultOptionsProvider::getRawOptions(FileName);
+  if (!RawOptions)
+return RawOptions;
   if (ConfigOptions.InheritParentConfig.value_or(false)) {
 LLVM_DEBUG(llvm::dbgs()
<< "Getting options for file " << FileName << "...\n");
@@ -303,13 +308,13 @@ ConfigOptionsProvider::getRawOptions(llvm::StringRef 
FileName) {
 llvm::ErrorOr> AbsoluteFilePath =
 getNormalizedAbsolutePath(FileName);
 if (AbsoluteFilePath) {
-  addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
+  addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
 }
   }
-  RawOptions.emplace_back(ConfigOptions,
-  OptionsSourceTypeConfigCommandLineOption);
-  RawOptions.emplace_back(OverrideOptions,
-  OptionsSourceTypeCheckCommandLineOption);
+  RawOptions->emplace_back(ConfigOptions,
+   OptionsSourceTypeConfigCommandLineOption);
+  RawOptions->emplace_back(OverrideOptions,
+   OptionsSourceTypeCheckCommandLineOption);
   return RawOptions;
 }
 
@@ -345,21 +350,21 @@ 
FileOptionsBaseProvider::getNormalizedAbsolutePath(llvm::StringRef Path) {
   return NormalizedAbsolutePath;
 }
 
-void FileOptionsBaseProvider::addRawFileOptions(
+std::error_code FileOptionsBaseProvider::addRawFileOptions(
 llvm::StringRef AbsolutePath, std::vector &CurOptions) {
   auto CurSize = CurOptions.size();
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
   auto Memori

[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)

2025-02-13 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/123010

>From 9d60d4980f1edbdd4cd4a9499f69e9d225717238 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Tue, 14 Jan 2025 20:44:10 -0800
Subject: [PATCH 1/5] Support BraceWrapping.AfterNamespace with
 AllowShortNamespacesOnASingleLine

AllowShortNamespacesOnASingleLine assumes that there is no newline
before the namespace brace, however, there is no actual reason this
shouldn't be compatible with BraceWrapping.AfterNamespace = true.

This is a little tricky in the implementation because
UnwrappedLineFormatter works on lines, so being flexible about the
offsets is awkard.

Not sure if there is a better pattern for combining the 'AllowShort'
options with the various configurations of BraceWrapping, but this
seemed mostly reasonable.  Really, it would almost be preferable to just
pattern match on the direct token stream, rathern than the
AnnotatedLines, but I'm not seeing a straightforward way to do that.
---
 clang/lib/Format/UnwrappedLineFormatter.cpp | 45 +
 clang/unittests/Format/FormatTest.cpp   | 20 +
 2 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp 
b/clang/lib/Format/UnwrappedLineFormatter.cpp
index cee84fb1191ab..787136a26b378 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -367,8 +367,12 @@ class LineJoiner {
 
 if (Style.AllowShortNamespacesOnASingleLine &&
 TheLine->First->is(tok::kw_namespace) &&
-TheLine->Last->is(tok::l_brace)) {
-  const auto result = tryMergeNamespace(I, E, Limit);
+((Style.BraceWrapping.AfterNamespace &&
+  NextLine.First->is(tok::l_brace)) ||
+ (!Style.BraceWrapping.AfterNamespace &&
+  TheLine->Last->is(tok::l_brace {
+  const auto result =
+  tryMergeNamespace(I, E, Limit, Style.BraceWrapping.AfterNamespace);
   if (result > 0)
 return result;
 }
@@ -628,28 +632,36 @@ class LineJoiner {
 
   unsigned tryMergeNamespace(ArrayRef::const_iterator I,
  ArrayRef::const_iterator E,
- unsigned Limit) {
+ unsigned Limit, bool OpenBraceWrapped) {
 if (Limit == 0)
   return 0;
 
-assert(I[1]);
-const auto &L1 = *I[1];
+// The merging code is relative to the opening namespace brace, which could
+// be either on the first or second line due to the brace wrapping rules.
+const size_t OpeningBraceLineOffset = OpenBraceWrapped ? 1 : 0;
+const auto BraceOpenLine = I + OpeningBraceLineOffset;
+
+if (std::distance(BraceOpenLine, E) <= 2)
+  return 0;
+
+if (BraceOpenLine[0]->Last->is(TT_LineComment))
+  return 0;
+
+assert(BraceOpenLine[1]);
+const auto &L1 = *BraceOpenLine[1];
 if (L1.InPPDirective != (*I)->InPPDirective ||
 (L1.InPPDirective && L1.First->HasUnescapedNewline)) {
   return 0;
 }
 
-if (std::distance(I, E) <= 2)
-  return 0;
-
-assert(I[2]);
-const auto &L2 = *I[2];
+assert(BraceOpenLine[2]);
+const auto &L2 = *BraceOpenLine[2];
 if (L2.Type == LT_Invalid)
   return 0;
 
 Limit = limitConsideringMacros(I + 1, E, Limit);
 
-if (!nextTwoLinesFitInto(I, Limit))
+if (!nextNLinesFitInto(I, I + OpeningBraceLineOffset + 2, Limit))
   return 0;
 
 // Check if it's a namespace inside a namespace, and call recursively if 
so.
@@ -660,17 +672,18 @@ class LineJoiner {
 
   assert(Limit >= L1.Last->TotalLength + 3);
   const auto InnerLimit = Limit - L1.Last->TotalLength - 3;
-  const auto MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+  const auto MergedLines =
+  tryMergeNamespace(BraceOpenLine + 1, E, InnerLimit, 
OpenBraceWrapped);
   if (MergedLines == 0)
 return 0;
-  const auto N = MergedLines + 2;
+  const auto N = MergedLines + 2 + OpeningBraceLineOffset;
   // Check if there is even a line after the inner result.
   if (std::distance(I, E) <= N)
 return 0;
   // Check that the line after the inner result starts with a closing brace
   // which we are permitted to merge into one line.
   if (I[N]->First->is(tok::r_brace) && !I[N]->First->MustBreakBefore &&
-  I[MergedLines + 1]->Last->isNot(tok::comment) &&
+  BraceOpenLine[MergedLines + 1]->Last->isNot(tok::comment) &&
   nextNLinesFitInto(I, I + N + 1, Limit)) {
 return N;
   }
@@ -688,8 +701,8 @@ class LineJoiner {
 if (L2.First->isNot(tok::r_brace) || L2.First->MustBreakBefore)
   return 0;
 
-// If so, merge all three lines.
-return 2;
+// If so, merge all lines.
+return 2 + OpeningBraceLineOffset;
   }
 
   unsigned
diff --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index 4d48bcacddead..bf008e61490f5 100644
--- a/clan

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-20 Thread Galen Elias via cfe-commits


galenelias wrote:

Ping

https://github.com/llvm/llvm-project/pull/136167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-27 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/136167

>From b8faa7bb02fb4aa0504978dad1a28493fc745786 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 16 Apr 2025 12:48:18 -0700
Subject: [PATCH 1/8] [clang-tidy] Return error code on config parse error

This addresses:

Issue 77341: clang-tidy fails to parse config files silently, even with
--verify-config.

Currently when clang-tidy attempts to parse a .clang-tidy config file
with invalid syntax, it emits and error and moves on, which can often
result in applying the default set of checks, which is undesirable.

This change bubbles up the error state from parsing the configuration
files up to `clangTidyMain` so we can do an early return with an error
code. It's unfortunately quite a bit of plumbing - but this seemed to be
the idiomatic approach for the clang-tidy codebase.
---
 .../ClangTidyDiagnosticConsumer.cpp   |  2 +-
 .../clang-tidy/ClangTidyOptions.cpp   | 66 +++
 .../clang-tidy/ClangTidyOptions.h | 23 ---
 .../clang-tidy/tool/ClangTidyMain.cpp | 47 +++--
 .../clang-tidy/OptionsProviderTest.cpp| 16 ++---
 5 files changed, 91 insertions(+), 63 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index b216970bfbd8c..0413c39bc58eb 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -266,7 +266,7 @@ ClangTidyOptions 
ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
   return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  *OptionsProvider->getOptions(File), 0);
 }
 
 void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index dd1d86882f5d4..7ae23c137106e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -267,16 +267,19 @@ const char
 ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] =
 "command-line option '-config'";
 
-ClangTidyOptions
+llvm::ErrorOr
 ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) {
   ClangTidyOptions Result;
   unsigned Priority = 0;
-  for (auto &Source : getRawOptions(FileName))
+  llvm::ErrorOr> Options = getRawOptions(FileName);
+  if (!Options)
+return Options.getError();
+  for (auto &Source : *Options)
 Result.mergeWith(Source.first, ++Priority);
   return Result;
 }
 
-std::vector
+llvm::ErrorOr>
 DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector Result;
   Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
@@ -292,10 +295,12 @@ ConfigOptionsProvider::ConfigOptionsProvider(
   std::move(OverrideOptions), std::move(FS)),
   ConfigOptions(std::move(ConfigOptions)) {}
 
-std::vector
+llvm::ErrorOr>
 ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
-  std::vector RawOptions =
+  llvm::ErrorOr> RawOptions =
   DefaultOptionsProvider::getRawOptions(FileName);
+  if (!RawOptions)
+return RawOptions;
   if (ConfigOptions.InheritParentConfig.value_or(false)) {
 LLVM_DEBUG(llvm::dbgs()
<< "Getting options for file " << FileName << "...\n");
@@ -303,13 +308,13 @@ ConfigOptionsProvider::getRawOptions(llvm::StringRef 
FileName) {
 llvm::ErrorOr> AbsoluteFilePath =
 getNormalizedAbsolutePath(FileName);
 if (AbsoluteFilePath) {
-  addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
+  addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
 }
   }
-  RawOptions.emplace_back(ConfigOptions,
-  OptionsSourceTypeConfigCommandLineOption);
-  RawOptions.emplace_back(OverrideOptions,
-  OptionsSourceTypeCheckCommandLineOption);
+  RawOptions->emplace_back(ConfigOptions,
+   OptionsSourceTypeConfigCommandLineOption);
+  RawOptions->emplace_back(OverrideOptions,
+   OptionsSourceTypeCheckCommandLineOption);
   return RawOptions;
 }
 
@@ -345,21 +350,21 @@ 
FileOptionsBaseProvider::getNormalizedAbsolutePath(llvm::StringRef Path) {
   return NormalizedAbsolutePath;
 }
 
-void FileOptionsBaseProvider::addRawFileOptions(
+std::error_code FileOptionsBaseProvider::addRawFileOptions(
 llvm::StringRef AbsolutePath, std::vector &CurOptions) {
   auto CurSize = CurOptions.size();
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
   auto Memori

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-27 Thread Galen Elias via cfe-commits



@@ -46,20 +46,65 @@ TEST(ClangTidyOptionsProvider, InMemoryFileSystems) {
 
   FileOptionsProvider FileOpt({}, {}, {}, FileSystem);
 
-  ClangTidyOptions File1Options =
+  llvm::ErrorOr File1Options =
   FileOpt.getOptions("ProjectRoot/SubDir1/File.cpp");
-  ClangTidyOptions File2Options =
+  llvm::ErrorOr File2Options =
   FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/File.cpp");
-  ClangTidyOptions File3Options =
+  llvm::ErrorOr File3Options =
   FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/SubDir3/File.cpp");
 
-  ASSERT_TRUE(File1Options.Checks.has_value());
-  EXPECT_EQ(*File1Options.Checks, "-*,clang-diagnostic-*,readability-*");
-  ASSERT_TRUE(File2Options.Checks.has_value());
-  EXPECT_EQ(*File2Options.Checks, "bugprone-*,misc-*,clang-diagnostic-*");
+  ASSERT_TRUE(File1Options->Checks.has_value());
+  EXPECT_EQ(*File1Options->Checks, "-*,clang-diagnostic-*,readability-*");
+  ASSERT_TRUE(File2Options->Checks.has_value());
+  EXPECT_EQ(*File2Options->Checks, "bugprone-*,misc-*,clang-diagnostic-*");
 
   // 2 and 3 should use the same config so these should also be the same.
-  EXPECT_EQ(File2Options.Checks, File3Options.Checks);
+  EXPECT_EQ(File2Options->Checks, File3Options->Checks);
+}
+
+TEST(ClangTidyOptionsProvider, InvalidConfigurationFiles) {
+  llvm::IntrusiveRefCntPtr FileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+
+  StringRef BaseClangTidy = R"(
+Checks: -*,clang-diagnostic-*
+Unexpected
+  )";
+  StringRef Sub1ClangTidy = R"(
+Checks: readability-*
+InheritParentConfig: true
+  )";
+  StringRef Sub2ClangTidy = R"(
+Checks: bugprone-*,misc-*,clang-diagnostic-*
+InheritParentConfig: false
+)";
+  FileSystem->addFile("ProjectRoot/.clang-tidy", 0,
+  llvm::MemoryBuffer::getMemBuffer(BaseClangTidy));
+  FileSystem->addFile("ProjectRoot/File.cpp", 0,
+  llvm::MemoryBuffer::getMemBuffer(""));
+  FileSystem->addFile("ProjectRoot/SubDir1/.clang-tidy", 0,
+  llvm::MemoryBuffer::getMemBuffer(Sub1ClangTidy));
+  FileSystem->addFile("ProjectRoot/SubDir1/File.cpp", 0,
+  llvm::MemoryBuffer::getMemBuffer(""));
+  FileSystem->addFile("ProjectRoot/SubDir1/SubDir2/.clang-tidy", 0,
+  llvm::MemoryBuffer::getMemBuffer(Sub2ClangTidy));
+  FileSystem->addFile("ProjectRoot/SubDir1/SubDir2/File.cpp", 0,
+  llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileOptionsProvider FileOpt({}, {}, {}, FileSystem);
+
+  llvm::ErrorOr File1Options =
+  FileOpt.getOptions("ProjectRoot/File.cpp");
+  llvm::ErrorOr File2Options =
+  FileOpt.getOptions("ProjectRoot/SubDir1/File.cpp");
+  llvm::ErrorOr File3Options =
+  FileOpt.getOptions("ProjectRoot/SubDir1/SubDir2/File.cpp");
+
+  ASSERT_TRUE(!File1Options);

galenelias wrote:

Yah, sorry - wasn't being very idiomatic here.  Cleaned them up.  I don't think 
we need the negations or double negations to coerce to booleans, it seems like 
we can just use the ASSERT_ macros directly.  Not sure why I formulated them 
this way.  Apologies.

https://github.com/llvm/llvm-project/pull/136167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-27 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/136167

>From b8faa7bb02fb4aa0504978dad1a28493fc745786 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 16 Apr 2025 12:48:18 -0700
Subject: [PATCH 1/7] [clang-tidy] Return error code on config parse error

This addresses:

Issue 77341: clang-tidy fails to parse config files silently, even with
--verify-config.

Currently when clang-tidy attempts to parse a .clang-tidy config file
with invalid syntax, it emits and error and moves on, which can often
result in applying the default set of checks, which is undesirable.

This change bubbles up the error state from parsing the configuration
files up to `clangTidyMain` so we can do an early return with an error
code. It's unfortunately quite a bit of plumbing - but this seemed to be
the idiomatic approach for the clang-tidy codebase.
---
 .../ClangTidyDiagnosticConsumer.cpp   |  2 +-
 .../clang-tidy/ClangTidyOptions.cpp   | 66 +++
 .../clang-tidy/ClangTidyOptions.h | 23 ---
 .../clang-tidy/tool/ClangTidyMain.cpp | 47 +++--
 .../clang-tidy/OptionsProviderTest.cpp| 16 ++---
 5 files changed, 91 insertions(+), 63 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index b216970bfbd8c..0413c39bc58eb 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -266,7 +266,7 @@ ClangTidyOptions 
ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
   return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  *OptionsProvider->getOptions(File), 0);
 }
 
 void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index dd1d86882f5d4..7ae23c137106e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -267,16 +267,19 @@ const char
 ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] =
 "command-line option '-config'";
 
-ClangTidyOptions
+llvm::ErrorOr
 ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) {
   ClangTidyOptions Result;
   unsigned Priority = 0;
-  for (auto &Source : getRawOptions(FileName))
+  llvm::ErrorOr> Options = getRawOptions(FileName);
+  if (!Options)
+return Options.getError();
+  for (auto &Source : *Options)
 Result.mergeWith(Source.first, ++Priority);
   return Result;
 }
 
-std::vector
+llvm::ErrorOr>
 DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector Result;
   Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
@@ -292,10 +295,12 @@ ConfigOptionsProvider::ConfigOptionsProvider(
   std::move(OverrideOptions), std::move(FS)),
   ConfigOptions(std::move(ConfigOptions)) {}
 
-std::vector
+llvm::ErrorOr>
 ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
-  std::vector RawOptions =
+  llvm::ErrorOr> RawOptions =
   DefaultOptionsProvider::getRawOptions(FileName);
+  if (!RawOptions)
+return RawOptions;
   if (ConfigOptions.InheritParentConfig.value_or(false)) {
 LLVM_DEBUG(llvm::dbgs()
<< "Getting options for file " << FileName << "...\n");
@@ -303,13 +308,13 @@ ConfigOptionsProvider::getRawOptions(llvm::StringRef 
FileName) {
 llvm::ErrorOr> AbsoluteFilePath =
 getNormalizedAbsolutePath(FileName);
 if (AbsoluteFilePath) {
-  addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
+  addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
 }
   }
-  RawOptions.emplace_back(ConfigOptions,
-  OptionsSourceTypeConfigCommandLineOption);
-  RawOptions.emplace_back(OverrideOptions,
-  OptionsSourceTypeCheckCommandLineOption);
+  RawOptions->emplace_back(ConfigOptions,
+   OptionsSourceTypeConfigCommandLineOption);
+  RawOptions->emplace_back(OverrideOptions,
+   OptionsSourceTypeCheckCommandLineOption);
   return RawOptions;
 }
 
@@ -345,21 +350,21 @@ 
FileOptionsBaseProvider::getNormalizedAbsolutePath(llvm::StringRef Path) {
   return NormalizedAbsolutePath;
 }
 
-void FileOptionsBaseProvider::addRawFileOptions(
+std::error_code FileOptionsBaseProvider::addRawFileOptions(
 llvm::StringRef AbsolutePath, std::vector &CurOptions) {
   auto CurSize = CurOptions.size();
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
   auto Memori

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-27 Thread Galen Elias via cfe-commits


https://github.com/galenelias updated 
https://github.com/llvm/llvm-project/pull/136167

>From b8faa7bb02fb4aa0504978dad1a28493fc745786 Mon Sep 17 00:00:00 2001
From: Galen Elias 
Date: Wed, 16 Apr 2025 12:48:18 -0700
Subject: [PATCH 1/8] [clang-tidy] Return error code on config parse error

This addresses:

Issue 77341: clang-tidy fails to parse config files silently, even with
--verify-config.

Currently when clang-tidy attempts to parse a .clang-tidy config file
with invalid syntax, it emits and error and moves on, which can often
result in applying the default set of checks, which is undesirable.

This change bubbles up the error state from parsing the configuration
files up to `clangTidyMain` so we can do an early return with an error
code. It's unfortunately quite a bit of plumbing - but this seemed to be
the idiomatic approach for the clang-tidy codebase.
---
 .../ClangTidyDiagnosticConsumer.cpp   |  2 +-
 .../clang-tidy/ClangTidyOptions.cpp   | 66 +++
 .../clang-tidy/ClangTidyOptions.h | 23 ---
 .../clang-tidy/tool/ClangTidyMain.cpp | 47 +++--
 .../clang-tidy/OptionsProviderTest.cpp| 16 ++---
 5 files changed, 91 insertions(+), 63 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index b216970bfbd8c..0413c39bc58eb 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -266,7 +266,7 @@ ClangTidyOptions 
ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
   return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  *OptionsProvider->getOptions(File), 0);
 }
 
 void ClangTidyContext::setEnableProfiling(bool P) { Profile = P; }
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index dd1d86882f5d4..7ae23c137106e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -267,16 +267,19 @@ const char
 ClangTidyOptionsProvider::OptionsSourceTypeConfigCommandLineOption[] =
 "command-line option '-config'";
 
-ClangTidyOptions
+llvm::ErrorOr
 ClangTidyOptionsProvider::getOptions(llvm::StringRef FileName) {
   ClangTidyOptions Result;
   unsigned Priority = 0;
-  for (auto &Source : getRawOptions(FileName))
+  llvm::ErrorOr> Options = getRawOptions(FileName);
+  if (!Options)
+return Options.getError();
+  for (auto &Source : *Options)
 Result.mergeWith(Source.first, ++Priority);
   return Result;
 }
 
-std::vector
+llvm::ErrorOr>
 DefaultOptionsProvider::getRawOptions(llvm::StringRef FileName) {
   std::vector Result;
   Result.emplace_back(DefaultOptions, OptionsSourceTypeDefaultBinary);
@@ -292,10 +295,12 @@ ConfigOptionsProvider::ConfigOptionsProvider(
   std::move(OverrideOptions), std::move(FS)),
   ConfigOptions(std::move(ConfigOptions)) {}
 
-std::vector
+llvm::ErrorOr>
 ConfigOptionsProvider::getRawOptions(llvm::StringRef FileName) {
-  std::vector RawOptions =
+  llvm::ErrorOr> RawOptions =
   DefaultOptionsProvider::getRawOptions(FileName);
+  if (!RawOptions)
+return RawOptions;
   if (ConfigOptions.InheritParentConfig.value_or(false)) {
 LLVM_DEBUG(llvm::dbgs()
<< "Getting options for file " << FileName << "...\n");
@@ -303,13 +308,13 @@ ConfigOptionsProvider::getRawOptions(llvm::StringRef 
FileName) {
 llvm::ErrorOr> AbsoluteFilePath =
 getNormalizedAbsolutePath(FileName);
 if (AbsoluteFilePath) {
-  addRawFileOptions(AbsoluteFilePath->str(), RawOptions);
+  addRawFileOptions(AbsoluteFilePath->str(), *RawOptions);
 }
   }
-  RawOptions.emplace_back(ConfigOptions,
-  OptionsSourceTypeConfigCommandLineOption);
-  RawOptions.emplace_back(OverrideOptions,
-  OptionsSourceTypeCheckCommandLineOption);
+  RawOptions->emplace_back(ConfigOptions,
+   OptionsSourceTypeConfigCommandLineOption);
+  RawOptions->emplace_back(OverrideOptions,
+   OptionsSourceTypeCheckCommandLineOption);
   return RawOptions;
 }
 
@@ -345,21 +350,21 @@ 
FileOptionsBaseProvider::getNormalizedAbsolutePath(llvm::StringRef Path) {
   return NormalizedAbsolutePath;
 }
 
-void FileOptionsBaseProvider::addRawFileOptions(
+std::error_code FileOptionsBaseProvider::addRawFileOptions(
 llvm::StringRef AbsolutePath, std::vector &CurOptions) {
   auto CurSize = CurOptions.size();
   // Look for a suitable configuration file in all parent directories of the
   // file. Start with the immediate parent directory and move up.
   StringRef RootPath = llvm::sys::path::parent_path(AbsolutePath);
   auto Memori

[clang-tools-extra] [clang-tidy] Return error code on config parse error (PR #136167)

2025-05-27 Thread Galen Elias via cfe-commits



@@ -265,8 +265,16 @@ const ClangTidyOptions &ClangTidyContext::getOptions() 
const {
 ClangTidyOptions ClangTidyContext::getOptionsForFile(StringRef File) const {
   // Merge options on top of getDefaults() as a safeguard against options with
   // unset values.
-  return ClangTidyOptions::getDefaults().merge(
-  OptionsProvider->getOptions(File), 0);
+  ClangTidyOptions defaultOptions = ClangTidyOptions::getDefaults();
+  llvm::ErrorOr fileOptions =
+  OptionsProvider->getOptions(File);
+
+  // If there was an error parsing the options, just use the default options.

galenelias wrote:

I agree, but it seemed quite difficult to propagate an error from this function 
and would drastically increase the size and invasiveness of this change, as 
every call of this function would need to propagate the error, and so on and so 
forth recursively until dozens+ of functions have been changed.

Given that in the primary clang-tidy usage scenario that these configs have 
already been pre-validated, and that this fallback behavior is basically 
equivalent to the previous behavior of silently continuing on with the default 
config, this seemed like a reasonable compromise.

I can pursue propagating the error, but when I initially investigated it, it 
seems quite complicated given the current places this method is called and how 
prepared the callers were for propagating errors.

https://github.com/llvm/llvm-project/pull/136167
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

47 matches

Mail list logo