Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions clang/docs/ClangFormatStyleOptions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1926,6 +1926,15 @@ the configuration (without a prefix: ``Auto``).
void f() {
}

* ``SFS_StaticInlineOnly`` (in configuration: ``StaticInlineOnly``)
Only merge functions defined as static inline.

.. code-block:: c++

void f5(void) {
}
static inline int f6(int a, int b) { return a + b; }

* ``SFS_Empty`` (in configuration: ``Empty``)
Only merge empty functions.

Expand All @@ -1949,6 +1958,14 @@ the configuration (without a prefix: ``Auto``).
}
void f() {}

* ``SFS_StaticInline`` (in configuration: ``StaticInline``)
Only merge functions defined as static inline. Implies ``empty``.

.. code-block:: c++

void f5(void) {}
static inline int f6(int a, int b) { return a + b; }

* ``SFS_All`` (in configuration: ``All``)
Merge all functions fitting on a single line.

Expand Down
13 changes: 13 additions & 0 deletions clang/include/clang/Format/Format.h
Original file line number Diff line number Diff line change
Expand Up @@ -844,6 +844,13 @@ struct FormatStyle {
/// }
/// \endcode
SFS_InlineOnly,
/// Only merge functions defined as static inline.
/// \code
/// void f5(void) {
/// }
/// static inline int f6(int a, int b) { return a + b; }
/// \endcode
SFS_StaticInlineOnly,
/// Only merge empty functions.
/// \code
/// void f() {}
Expand All @@ -863,6 +870,12 @@ struct FormatStyle {
/// void f() {}
/// \endcode
SFS_Inline,
/// Only merge functions defined as static inline. Implies ``empty``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have a different text.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed. Now it mention that empty function also merged.

/// \code
/// void f5(void) {}
/// static inline int f6(int a, int b) { return a + b; }
/// \endcode
SFS_StaticInline,
/// Merge all functions fitting on a single line.
/// \code
/// class Foo {
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Format/Format.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,8 @@ template <> struct ScalarEnumerationTraits<FormatStyle::ShortFunctionStyle> {
IO.enumCase(Value, "Inline", FormatStyle::SFS_Inline);
IO.enumCase(Value, "InlineOnly", FormatStyle::SFS_InlineOnly);
IO.enumCase(Value, "Empty", FormatStyle::SFS_Empty);
IO.enumCase(Value, "StaticInlineOnly", FormatStyle::SFS_StaticInlineOnly);
IO.enumCase(Value, "StaticInline", FormatStyle::SFS_StaticInline);
}
};

Expand Down
6 changes: 4 additions & 2 deletions clang/lib/Format/TokenAnnotator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5690,8 +5690,10 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
return Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_None ||
Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Empty ||
(Left.NestingLevel == 0 && Line.Level == 0 &&
Style.AllowShortFunctionsOnASingleLine &
FormatStyle::SFS_InlineOnly);
(Style.AllowShortFunctionsOnASingleLine ==
FormatStyle::SFS_InlineOnly ||
Style.AllowShortFunctionsOnASingleLine ==
FormatStyle::SFS_Inline));
}
} else if (Style.Language == FormatStyle::LK_Java) {
if (Right.is(tok::plus) && Left.is(tok::string_literal) && AfterRight &&
Expand Down
31 changes: 29 additions & 2 deletions clang/lib/Format/UnwrappedLineFormatter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -300,8 +300,9 @@ class LineJoiner {
return true;
}

if (Style.AllowShortFunctionsOnASingleLine &
FormatStyle::SFS_InlineOnly) {
if (Style.AllowShortFunctionsOnASingleLine ==
FormatStyle::SFS_InlineOnly ||
Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline) {
// Just checking TheLine->Level != 0 is not enough, because it
// provokes treating functions inside indented namespaces as short.
if (Style.isJavaScript() && TheLine->Last->is(TT_FunctionLBrace))
Expand Down Expand Up @@ -335,6 +336,32 @@ class LineJoiner {
}
}

if (Style.AllowShortFunctionsOnASingleLine ==
FormatStyle::SFS_StaticInlineOnly ||
Style.AllowShortFunctionsOnASingleLine ==
FormatStyle::SFS_StaticInline) {
// Check if the current line belongs to a static inline function
const auto *FirstNonCommentToken =
TheLine ? TheLine->getFirstNonComment() : nullptr;

// Look for 'static' and 'inline' keywords in any order
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Look for 'static' and 'inline' keywords in any order
// Look for 'static' and 'inline' keywords in any order.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed

bool HasStatic = false;
bool HasInline = false;
const FormatToken *Tok = FirstNonCommentToken;

while (Tok && !Tok->is(TT_FunctionLBrace)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You really don't need to go until the brace, the paren would suffice, or even better the name?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the name is sufficient. Thanks for idea. I've made a fix; please take a look.

if (Tok->is(tok::kw_static))
HasStatic = true;
if (Tok->is(tok::kw_inline))
HasInline = true;
Tok = Tok->Next;
}

// If we found both static and inline, allow merging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If we found both static and inline, allow merging
// If we found both static and inline, allow merging.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed

if (HasStatic && HasInline)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this check into the ifs of the loop, exit early.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added additional check in while(). Please take a look.

return true;
}

return false;
};

Expand Down
6 changes: 6 additions & 0 deletions clang/unittests/Format/ConfigParseTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,12 @@ TEST(ConfigParseTest, ParsesConfiguration) {
AllowShortFunctionsOnASingleLine, FormatStyle::SFS_Empty);
CHECK_PARSE("AllowShortFunctionsOnASingleLine: All",
AllowShortFunctionsOnASingleLine, FormatStyle::SFS_All);
CHECK_PARSE("AllowShortFunctionsOnASingleLine: StaticInlineOnly",
AllowShortFunctionsOnASingleLine,
FormatStyle::SFS_StaticInlineOnly);
CHECK_PARSE("AllowShortFunctionsOnASingleLine: StaticInline",
AllowShortFunctionsOnASingleLine, FormatStyle::SFS_StaticInline);

// For backward compatibility:
CHECK_PARSE("AllowShortFunctionsOnASingleLine: false",
AllowShortFunctionsOnASingleLine, FormatStyle::SFS_None);
Expand Down
79 changes: 79 additions & 0 deletions clang/unittests/Format/FormatTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15120,6 +15120,85 @@ TEST_F(FormatTest, PullInlineFunctionDefinitionsIntoSingleLine) {
MergeInlineOnly);
}

TEST_F(FormatTest, PullStaticInlineFunctionDefinitionsIntoSingleLine) {
FormatStyle MergeStaticInlineOnly = getLLVMStyle();
MergeStaticInlineOnly.AllowShortFunctionsOnASingleLine =
FormatStyle::SFS_StaticInlineOnly;
verifyFormat("static inline int f() { return 42; }",
"static inline int f() {\n"
" return 42; \n"
"}",
MergeStaticInlineOnly);
verifyFormat("inline static int f() { return 42; }",
"inline static int f() \n"
"{\n"
" return 42; \n"
"}",
MergeStaticInlineOnly);
verifyNoChange("int f() {\n"
" return 42;\n"
"}",
MergeStaticInlineOnly);

verifyFormat("void f1(void) {\n"
"}",
"void f1(void)\n"
"{\n"
"}",
MergeStaticInlineOnly);

verifyNoChange("int f2(int a, int b) {\n"
" return a + b;\n"
"}",
MergeStaticInlineOnly);

verifyNoChange("static void f3(void) {\n"
"}\n",
MergeStaticInlineOnly);

verifyFormat("static int f4(int a, int b) {\n"
" return a + b;\n"
"}\n",
MergeStaticInlineOnly);

verifyNoChange("static inline void f5(void) {}", MergeStaticInlineOnly);

verifyFormat("static inline int f6(int a, int b) { return a + b; }",
"static inline int f6(int a, int b) \n"
"{ return a + b; }",
MergeStaticInlineOnly);

verifyFormat("int f(int a, int b) {\n"
" return a + b;\n"
"}",
"int f(int a, int b) { return a + b; }", MergeStaticInlineOnly);

FormatStyle MergeStaticInline = getLLVMStyle();
MergeStaticInline.AllowShortFunctionsOnASingleLine =
FormatStyle::SFS_StaticInline;
verifyFormat("static inline int f() { return 42; }",
"static inline int f() {\n"
" return 42; \n"
"}",
MergeStaticInline);
verifyFormat("inline static int f() { return 42; }",
"inline static int f() \n"
"{\n"
" return 42; \n"
"}",
MergeStaticInline);
verifyNoChange("int f() {\n"
" return 42;\n"
"}",
MergeStaticInline);

verifyFormat("void f1(void) {}",
"void f1(void)\n"
"{\n"
"}",
MergeStaticInline);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests with attributes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some tests with different attributes in different places. If you have any idea what should be added - small example (just code line) would be appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about constexpr this implies inline, what should be done in this case?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no preference regarding constexpr because it was not mentioned in 'Inline' or 'InlineOnly' options before. Right now it won't be formatted if StaticInline is specified (I added unit test to show this). Please let me know if I need to change this.


TEST_F(FormatTest, PullInlineOnlyFunctionDefinitionsIntoSingleLine) {
FormatStyle MergeInlineOnly = getLLVMStyle();
MergeInlineOnly.AllowShortFunctionsOnASingleLine =
Expand Down