aybassiouny created this revision.
aybassiouny added reviewers: curdeius, Wawha, MyDeveloperDay,
HazardyKnusperkeks.
aybassiouny added a project: clang-format.
aybassiouny requested review of this revision.
Herald added a project: clang.
**Summary** `CompactNamespaces` config stops work when
`AllowShortLambdasOnASingleLine` is set to false or
`BraceWrapping.BeforeLambdaBody` is true
Input:
namespace out {
namespace in {
}
} // namespace out::in
Expected output:
namespace out { namespace in {
}} // namespace out::in
Output from current binaries:
namespace out {
namespace in {
}
} // namespace out::in
Config triggering the issue:
---
AllowShortLambdasOnASingleLine: None
CompactNamespaces: true
...
This config too triggers the same issue:
---
BraceWrapping:
BeforeLambdaBody : true
BreakBeforeBraces: Custom
CompactNamespaces: true
...
**What's happening:** Seems there's a corner case when
AllowShortLambdasOnASingleLine is false, or when
`BraceWrapping.BeforeLambdaBody` is true, that causes CompactNamespaces to stop
working. The root cause seems to be isAllmanLambdaBrace
<https://github.com/llvm/llvm-project/blob/bcc1dee60019f3a488a04dc7f701f7a692040fed/clang/lib/Format/TokenAnnotator.cpp#L3495>,
that !!assumes the namespace brace is a lambda and blocks compacting the
namespace!!. The regression was probably introduced with this commit
<https://github.com/llvm/llvm-project/commit/fa0118e6e588fe303b08e7e06ba28ac1f8d50c68>.
I have only added a UT covering the corner cases, please let me know if more
coverage is needed, I am new to the codebase. One corner case I am aware of, is
anonymous namespaces, which would not be affected by my
`Tok.Previous->Previous->is(tok::kw_namespace)` check, but I am not sure what's
the expected behavior there anyway.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D99031
Files:
clang/lib/Format/TokenAnnotator.cpp
clang/unittests/Format/FormatTest.cpp
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2589,6 +2589,33 @@
Style));
}
+TEST_F(FormatTest, FormatsCompactNamespacesLambdaRegression) {
+ FormatStyle Style = getLLVMStyle();
+ Style.CompactNamespaces = true;
+
+ // Make sure compact namespaces are not confused with lambdas
+ auto NoShortLambdaStyle{Style};
+ NoShortLambdaStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_None;
+ EXPECT_EQ("namespace out { namespace in {\n"
+ "}} // namespace out::in",
+ format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ NoShortLambdaStyle));
+
+ auto BraceWrappingBeforeLambdaBodyStyle{Style};
+ BraceWrappingBeforeLambdaBodyStyle.BreakBeforeBraces =
FormatStyle::BS_Custom;
+ BraceWrappingBeforeLambdaBodyStyle.BraceWrapping.BeforeLambdaBody = true;
+ EXPECT_EQ("namespace out { namespace in {\n"
+ "}} // namespace out::in",
+ format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ BraceWrappingBeforeLambdaBodyStyle));
+}
+
TEST_F(FormatTest, FormatsExternC) {
verifyFormat("extern \"C\" {\nint a;");
verifyFormat("extern \"C\" {}");
Index: clang/lib/Format/TokenAnnotator.cpp
===================================================================
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3494,7 +3494,8 @@
}
static bool isAllmanLambdaBrace(const FormatToken &Tok) {
return (Tok.is(tok::l_brace) && Tok.is(BK_Block) &&
- !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral));
+ !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral) &&
+ !Tok.Previous->Previous->is(tok::kw_namespace));
}
static bool isAllmanBraceIncludedBreakableLambda(
Index: clang/unittests/Format/FormatTest.cpp
===================================================================
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2589,6 +2589,33 @@
Style));
}
+TEST_F(FormatTest, FormatsCompactNamespacesLambdaRegression) {
+ FormatStyle Style = getLLVMStyle();
+ Style.CompactNamespaces = true;
+
+ // Make sure compact namespaces are not confused with lambdas
+ auto NoShortLambdaStyle{Style};
+ NoShortLambdaStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_None;
+ EXPECT_EQ("namespace out { namespace in {\n"
+ "}} // namespace out::in",
+ format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ NoShortLambdaStyle));
+
+ auto BraceWrappingBeforeLambdaBodyStyle{Style};
+ BraceWrappingBeforeLambdaBodyStyle.BreakBeforeBraces = FormatStyle::BS_Custom;
+ BraceWrappingBeforeLambdaBodyStyle.BraceWrapping.BeforeLambdaBody = true;
+ EXPECT_EQ("namespace out { namespace in {\n"
+ "}} // namespace out::in",
+ format("namespace out {\n"
+ "namespace in {\n"
+ "} // namespace in\n"
+ "} // namespace out",
+ BraceWrappingBeforeLambdaBodyStyle));
+}
+
TEST_F(FormatTest, FormatsExternC) {
verifyFormat("extern \"C\" {\nint a;");
verifyFormat("extern \"C\" {}");
Index: clang/lib/Format/TokenAnnotator.cpp
===================================================================
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3494,7 +3494,8 @@
}
static bool isAllmanLambdaBrace(const FormatToken &Tok) {
return (Tok.is(tok::l_brace) && Tok.is(BK_Block) &&
- !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral));
+ !Tok.isOneOf(TT_ObjCBlockLBrace, TT_DictLiteral) &&
+ !Tok.Previous->Previous->is(tok::kw_namespace));
}
static bool isAllmanBraceIncludedBreakableLambda(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
