[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
This commit is contained in:
Marek Kurdej 2022-01-14 21:51:06 +01:00
parent e0841f6920
commit 7af11989be
4 changed files with 170 additions and 8 deletions

View File

@ -262,14 +262,58 @@ private:
}
}
// 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()) {

View File

@ -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"

View File

@ -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

View File

@ -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