[clang] 7af1198 - [clang-format] Fix short functions being considered as inline inside an indented namespace.
Marek Kurdej via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 14 12:57:05 PST 2022
Author: Marek Kurdej
Date: 2022-01-14T21:57:02+01:00
New Revision: 7af11989be21df00ad6a510a831ea2425e48fa90
URL: https://github.com/llvm/llvm-project/commit/7af11989be21df00ad6a510a831ea2425e48fa90
DIFF: https://github.com/llvm/llvm-project/commit/7af11989be21df00ad6a510a831ea2425e48fa90.diff
LOG: [clang-format] Fix short functions being considered as inline inside an indented namespace.
Fixes https://github.com/llvm/llvm-project/issues/24784.
With config:
```
AllowShortFunctionsOnASingleLine: Inline
NamespaceIndentation: All
```
The code:
```
namespace Test
{
void f()
{
return;
}
}
```
was incorrectly formatted to:
```
namespace Test
{
void f() { return; }
}
```
since the function `f` was considered being inside a class/struct/record.
That's because the check was simplistic and only checked for a non-zero indentation level of the line starting `f`.
Reviewed By: MyDeveloperDay, HazardyKnusperkeks
Differential Revision: https://reviews.llvm.org/D117142
Added:
Modified:
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/unittests/Format/FormatTest.cpp
clang/unittests/Format/FormatTestCSharp.cpp
clang/unittests/Format/FormatTestJava.cpp
Removed:
################################################################################
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 18d61b4839526..7c447bbfaa5d3 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -262,14 +262,58 @@ class LineJoiner {
}
}
- // FIXME: TheLine->Level != 0 might or might not be the right check to do.
- // If necessary, change to something smarter.
- bool MergeShortFunctions =
- Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All ||
- (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
- I[1]->First->is(tok::r_brace)) ||
- (Style.AllowShortFunctionsOnASingleLine & FormatStyle::SFS_InlineOnly &&
- TheLine->Level != 0);
+ auto ShouldMergeShortFunctions =
+ [this, B = AnnotatedLines.begin()](
+ SmallVectorImpl<AnnotatedLine *>::const_iterator I) {
+ if (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All)
+ return true;
+ if (Style.AllowShortFunctionsOnASingleLine >=
+ FormatStyle::SFS_Empty &&
+ I[1]->First->is(tok::r_brace))
+ return true;
+
+ if (Style.AllowShortFunctionsOnASingleLine &
+ FormatStyle::SFS_InlineOnly) {
+ // Just checking TheLine->Level != 0 is not enough, because it
+ // provokes treating functions inside indented namespaces as short.
+ if ((*I)->Level != 0) {
+ if (I == B)
+ return false;
+
+ // TODO: Use IndentTracker to avoid loop?
+ // Find the last line with lower level.
+ auto J = I - 1;
+ for (; J != B; --J)
+ if ((*J)->Level < (*I)->Level)
+ break;
+
+ // Check if the found line starts a record.
+ auto *RecordTok = (*J)->First;
+ while (RecordTok) {
+ // TODO: Refactor to isRecord(RecordTok).
+ if (RecordTok->isOneOf(tok::kw_class, tok::kw_struct))
+ return true;
+ if (Style.isCpp() && RecordTok->is(tok::kw_union))
+ return true;
+ if (Style.isCSharp() && RecordTok->is(Keywords.kw_interface))
+ return true;
+ if (Style.Language == FormatStyle::LK_Java &&
+ RecordTok->is(tok::kw_enum))
+ return true;
+ if (Style.isJavaScript() && RecordTok->is(Keywords.kw_function))
+ return true;
+
+ RecordTok = RecordTok->Next;
+ }
+
+ return false;
+ }
+ }
+
+ return false;
+ };
+
+ bool MergeShortFunctions = ShouldMergeShortFunctions(I);
if (Style.CompactNamespaces) {
if (auto nsToken = TheLine->First->getNamespaceToken()) {
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index eb6975bbe77e8..27bf1d14e6664 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -3560,6 +3560,86 @@ TEST_F(FormatTest, FormatsNamespaces) {
"} // namespace out",
Style));
+ FormatStyle ShortInlineFunctions = getLLVMStyle();
+ ShortInlineFunctions.NamespaceIndentation = FormatStyle::NI_All;
+ ShortInlineFunctions.AllowShortFunctionsOnASingleLine =
+ FormatStyle::SFS_Inline;
+ verifyFormat("namespace {\n"
+ " void f() {\n"
+ " return;\n"
+ " }\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace {\n"
+ " int some_int;\n"
+ " void f() {\n"
+ " return;\n"
+ " }\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace interface {\n"
+ " void f() {\n"
+ " return;\n"
+ " }\n"
+ "} // namespace interface\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace {\n"
+ " class X {\n"
+ " void f() { return; }\n"
+ " };\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace {\n"
+ " struct X {\n"
+ " void f() { return; }\n"
+ " };\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace {\n"
+ " union X {\n"
+ " void f() { return; }\n"
+ " };\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("extern \"C\" {\n"
+ "void f() {\n"
+ " return;\n"
+ "}\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace {\n"
+ " class X {\n"
+ " void f() { return; }\n"
+ " } x;\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace {\n"
+ " [[nodiscard]] class X {\n"
+ " void f() { return; }\n"
+ " };\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace {\n"
+ " static class X {\n"
+ " void f() { return; }\n"
+ " } x;\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+ verifyFormat("namespace {\n"
+ " constexpr class X {\n"
+ " void f() { return; }\n"
+ " } x;\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+
+ ShortInlineFunctions.IndentExternBlock = FormatStyle::IEBS_Indent;
+ verifyFormat("extern \"C\" {\n"
+ " void f() {\n"
+ " return;\n"
+ " }\n"
+ "} // namespace\n",
+ ShortInlineFunctions);
+
Style.NamespaceIndentation = FormatStyle::NI_Inner;
EXPECT_EQ("namespace out {\n"
"int i;\n"
diff --git a/clang/unittests/Format/FormatTestCSharp.cpp b/clang/unittests/Format/FormatTestCSharp.cpp
index 2cfec61d8f94b..26c62c145064d 100644
--- a/clang/unittests/Format/FormatTestCSharp.cpp
+++ b/clang/unittests/Format/FormatTestCSharp.cpp
@@ -1518,5 +1518,32 @@ TEST_F(FormatTestCSharp, EmptyShortBlock) {
Style);
}
+TEST_F(FormatTestCSharp, ShortFunctions) {
+ FormatStyle Style = getLLVMStyle(FormatStyle::LK_CSharp);
+ Style.NamespaceIndentation = FormatStyle::NI_All;
+ Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
+ verifyFormat("interface Interface {\n"
+ " void f() { return; }\n"
+ "};",
+ Style);
+ verifyFormat("public interface Interface {\n"
+ " void f() { return; }\n"
+ "};",
+ Style);
+ verifyFormat("namespace {\n"
+ " void f() {\n"
+ " return;\n"
+ " }\n"
+ "};",
+ Style);
+ // "union" is not a keyword in C#.
+ verifyFormat("namespace union {\n"
+ " void f() {\n"
+ " return;\n"
+ " }\n"
+ "};",
+ Style);
+}
+
} // namespace format
} // end namespace clang
diff --git a/clang/unittests/Format/FormatTestJava.cpp b/clang/unittests/Format/FormatTestJava.cpp
index 84f6420b9e1ac..e778836e0fc9a 100644
--- a/clang/unittests/Format/FormatTestJava.cpp
+++ b/clang/unittests/Format/FormatTestJava.cpp
@@ -602,5 +602,16 @@ TEST_F(FormatTestJava, RetainsLogicalShifts) {
"}");
}
+TEST_F(FormatTestJava, ShortFunctions) {
+ FormatStyle Style = getLLVMStyle(FormatStyle::LK_Java);
+ Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
+ verifyFormat("enum Enum {\n"
+ " E1,\n"
+ " E2;\n"
+ " void f() { return; }\n"
+ "}",
+ Style);
+}
+
} // namespace format
} // end namespace clang
More information about the cfe-commits
mailing list