-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[clang-format]: Add Custom
to ShortFunctionStyle
; add new AllowShortFunctionsOnASingleLineOptions for granular setup
#134337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ortFunctionsOnASingleLineOptions for granular setup The current clang-format configuration option AllowShortFunctionsOnASingleLine uses a single enum (ShortFunctionStyle) to control when short function definitions can be merged onto a single line. This enum provides predefined combinations of conditions (e.g., None, Empty only, Inline only, Inline including Empty, All). This approach has limitations: 1. **Lack of Granularity:** Users cannot specify arbitrary combinations of conditions. For example, a user might want to allow merging for both empty functions and short top-level functions, but not for short functions defined within classes. This is not possible with the current enum options except by choosing All, which might merge more than desired. 2. **Inflexibility:** Adding new conditions for merging (e.g., distinguishing between member functions and constructors, handling lambdas specifically) would require adding many new combined enum values, leading to a combinatorial explosion and making the configuration complex. 3. **Implicit Behavior:** Some options imply others (e.g., Inline implies Empty), which might not always be intuitive or desired. The goal is to replace this single-choice enum with a more flexible mechanism allowing users to specify a set of conditions that must be met for a short function to be merged onto a single line. **Proposed Solution** 1. Introduce a new configuration option: AllowShortFunctionsOnSingleLineOptions. 2. This option will accept a list of strings, where each string represents a specific condition allowing merging. 3. **Backward Compatibility:** - If AllowShortFunctionsOnSingleLineOptions is present in the configuration, it takes precedence. - If AllowShortFunctionsOnSingleLineOptions is not present, but the old AllowShortFunctionsOnASingleLine is present, the old option should be parsed and mapped to the corresponding new semantics for compatibility.
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-format Author: Ivan (irymarchyk) ChangesThe current clang-format configuration option AllowShortFunctionsOnASingleLine uses a single enum (ShortFunctionStyle) to control when short function definitions can be merged onto a single line. This enum provides predefined combinations of conditions (e.g., None, Empty only, Inline only, Inline including Empty, All). This approach has limitations:
The goal is to replace this single-choice enum with a more flexible mechanism allowing users to specify a set of conditions that must be met for a short function to be merged onto a single line. Proposed Solution
Patch is 21.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/134337.diff 7 Files Affected:
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 9ecac68ae72bf..167701cf6585d 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -1959,6 +1959,70 @@ the configuration (without a prefix: ``Auto``).
};
void f() { bar(); }
+ * ``SFS_Custom`` (in configuration: ``Custom``)
+ Configure merge behavior using AllowShortFunctionsOnASingleLineOptions
+
+
+
+.. _AllowShortFunctionsOnASingleLineOptions:
+
+**AllowShortFunctionsOnASingleLineOptions** (``ShortFunctionMergeFlags``) :versionbadge:`clang-format 21` :ref:`¶ <AllowShortFunctionsOnASingleLineOptions>`
+ Precise control over merging short functions
+
+ If ``AllowShortFunctionsOnASingleLine`` is set to ``Custom``, use this to
+ specify behavior in different situations.
+
+ .. code-block:: yaml
+
+ # Example of usage:
+ AllowShortFunctionsOnASingleLine: Custom
+ AllowShortFunctionsOnASingleLineOptions:
+ Empty: false
+ Inline: true
+ All: false
+
+ Nested configuration flags:
+
+ Precise control over merging short functions
+
+ .. code-block:: c++
+
+ # Should be declared this way:
+ AllowShortFunctionsOnASingleLine: Custom
+ AllowShortFunctionsOnASingleLineOptions:
+ Empty: false
+ Inline: true
+ All: false
+
+ * ``bool Empty`` Only merge empty functions.
+
+ .. code-block:: c++
+
+ void f() {}
+ void f2() {
+ bar2();
+ }
+
+ * ``bool Inline`` Only merge functions defined inside a class.
+
+ .. code-block:: c++
+
+ class Foo {
+ void f() { foo(); }
+ };
+ void f() {
+ foo();
+ }
+ void f() {}
+
+ * ``bool All`` Merge all functions fitting on a single line.
+
+ .. code-block:: c++
+
+ class Foo {
+ void f() { foo(); }
+ };
+ void f() { bar(); }
.. _AllowShortIfStatementsOnASingleLine:
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index fec47a248abb4..96b1ecab04e63 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -871,6 +871,8 @@ struct FormatStyle {
/// void f() { bar(); }
/// \endcode
SFS_All,
+ /// Configure merge behavior using AllowShortFunctionsOnASingleLineOptions
+ SFS_Custom,
};
/// Dependent on the value, ``int f() { return 0; }`` can be put on a
@@ -878,6 +880,72 @@ struct FormatStyle {
/// \version 3.5
ShortFunctionStyle AllowShortFunctionsOnASingleLine;
+ /// Precise control over merging short functions
+ /// \code
+ /// # Should be declared this way:
+ /// AllowShortFunctionsOnASingleLine: Custom
+ /// AllowShortFunctionsOnASingleLineOptions:
+ /// Empty: false
+ /// Inline: true
+ /// All: false
+ /// \endcode
+ struct ShortFunctionMergeFlags {
+ /// Only merge empty functions.
+ /// \code
+ /// void f() {}
+ /// void f2() {
+ /// bar2();
+ /// }
+ /// \endcode
+ bool Empty;
+ /// Only merge functions defined inside a class.
+ /// \code
+ /// class Foo {
+ /// void f() { foo(); }
+ /// };
+ /// void f() {
+ /// foo();
+ /// }
+ /// void f() {}
+ /// \endcode
+ bool Inline;
+ /// Merge all functions fitting on a single line.
+ /// \code
+ /// class Foo {
+ /// void f() { foo(); }
+ /// };
+ /// void f() { bar(); }
+ /// \endcode
+ bool All;
+
+ ShortFunctionMergeFlags() : Empty(false), Inline(false), All(false) {}
+
+ ShortFunctionMergeFlags(bool Empty, bool Inline, bool All)
+ : Empty(Empty), Inline(Inline), All(All) {}
+
+ bool operator==(const ShortFunctionMergeFlags &R) const {
+ return Empty == R.Empty && Inline == R.Inline && All == R.All;
+ }
+ bool operator!=(const ShortFunctionMergeFlags &R) const {
+ return !(*this == R);
+ }
+ };
+
+ /// Precise control over merging short functions
+ ///
+ /// If ``AllowShortFunctionsOnASingleLine`` is set to ``Custom``, use this to
+ /// specify behavior in different situations.
+ /// \code{.yaml}
+ /// # Example of usage:
+ /// AllowShortFunctionsOnASingleLine: Custom
+ /// AllowShortFunctionsOnASingleLineOptions:
+ /// Empty: false
+ /// Inline: true
+ /// All: false
+ /// \endcode
+ /// \version 21
+ ShortFunctionMergeFlags AllowShortFunctionsOnASingleLineOptions;
+
/// Different styles for handling short if statements.
enum ShortIfStyle : int8_t {
/// Never put short ifs on the same line.
@@ -5281,6 +5349,8 @@ struct FormatStyle {
AllowShortEnumsOnASingleLine == R.AllowShortEnumsOnASingleLine &&
AllowShortFunctionsOnASingleLine ==
R.AllowShortFunctionsOnASingleLine &&
+ AllowShortFunctionsOnASingleLineOptions ==
+ R.AllowShortFunctionsOnASingleLineOptions &&
AllowShortIfStatementsOnASingleLine ==
R.AllowShortIfStatementsOnASingleLine &&
AllowShortLambdasOnASingleLine == R.AllowShortLambdasOnASingleLine &&
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 28aea86139e0d..21f896c6e9cf0 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -622,6 +622,15 @@ 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, "Custom", FormatStyle::SFS_Custom);
+ }
+};
+
+template <> struct MappingTraits<FormatStyle::ShortFunctionMergeFlags> {
+ static void mapping(IO &IO, FormatStyle::ShortFunctionMergeFlags &Value) {
+ IO.mapOptional("Empty", Value.Empty);
+ IO.mapOptional("Inline", Value.Inline);
+ IO.mapOptional("All", Value.All);
}
};
@@ -982,6 +991,8 @@ template <> struct MappingTraits<FormatStyle> {
Style.AllowShortEnumsOnASingleLine);
IO.mapOptional("AllowShortFunctionsOnASingleLine",
Style.AllowShortFunctionsOnASingleLine);
+ IO.mapOptional("AllowShortFunctionsOnASingleLineOptions",
+ Style.AllowShortFunctionsOnASingleLineOptions);
IO.mapOptional("AllowShortIfStatementsOnASingleLine",
Style.AllowShortIfStatementsOnASingleLine);
IO.mapOptional("AllowShortLambdasOnASingleLine",
@@ -1472,6 +1483,30 @@ static void expandPresetsSpacesInParens(FormatStyle &Expanded) {
Expanded.SpacesInParensOptions = {};
}
+static void expandPresetsShortFunctionsOnSingleLine(FormatStyle &Expanded) {
+ if (Expanded.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Custom)
+ return;
+ // Reset all flags
+ Expanded.AllowShortFunctionsOnASingleLineOptions = {};
+
+ if (Expanded.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_None)
+ return;
+
+ if (Expanded.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Empty ||
+ Expanded.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline) {
+ Expanded.AllowShortFunctionsOnASingleLineOptions.Empty = true;
+ }
+
+ if (Expanded.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Inline ||
+ Expanded.AllowShortFunctionsOnASingleLine ==
+ FormatStyle::SFS_InlineOnly) {
+ Expanded.AllowShortFunctionsOnASingleLineOptions.Inline = true;
+ }
+
+ if (Expanded.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All)
+ Expanded.AllowShortFunctionsOnASingleLineOptions.All = true;
+}
+
FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
FormatStyle LLVMStyle;
LLVMStyle.AccessModifierOffset = -2;
@@ -1501,6 +1536,8 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true;
LLVMStyle.AllowShortEnumsOnASingleLine = true;
LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All;
+ LLVMStyle.AllowShortFunctionsOnASingleLineOptions = {};
+ LLVMStyle.AllowShortFunctionsOnASingleLineOptions.All = true;
LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
LLVMStyle.AllowShortLoopsOnASingleLine = false;
@@ -1788,6 +1825,8 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
GoogleStyle.AlignTrailingComments = {};
GoogleStyle.AlignTrailingComments.Kind = FormatStyle::TCAS_Never;
GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+ GoogleStyle.AllowShortFunctionsOnASingleLineOptions = {};
+ GoogleStyle.AllowShortFunctionsOnASingleLineOptions.Empty = true;
GoogleStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
GoogleStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
@@ -1798,6 +1837,8 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
GoogleStyle.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
GoogleStyle.AlignOperands = FormatStyle::OAS_DontAlign;
GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+ GoogleStyle.AllowShortFunctionsOnASingleLineOptions = {};
+ GoogleStyle.AllowShortFunctionsOnASingleLineOptions.Empty = true;
// TODO: still under discussion whether to switch to SLS_All.
GoogleStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_Empty;
GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
@@ -1815,6 +1856,8 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
GoogleStyle.SpacesInContainerLiterals = false;
} else if (Language == FormatStyle::LK_Proto) {
GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+ GoogleStyle.AllowShortFunctionsOnASingleLineOptions = {};
+ GoogleStyle.AllowShortFunctionsOnASingleLineOptions.Empty = true;
GoogleStyle.AlwaysBreakBeforeMultilineStrings = false;
// This affects protocol buffer options specifications and text protos.
// Text protos are currently mostly formatted inside C++ raw string literals
@@ -1834,6 +1877,8 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
tooling::IncludeStyle::IBS_Preserve;
} else if (Language == FormatStyle::LK_CSharp) {
GoogleStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty;
+ GoogleStyle.AllowShortFunctionsOnASingleLineOptions = {};
+ GoogleStyle.AllowShortFunctionsOnASingleLineOptions.Empty = true;
GoogleStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
GoogleStyle.BreakStringLiterals = false;
GoogleStyle.ColumnLimit = 100;
@@ -1893,6 +1938,8 @@ FormatStyle getChromiumStyle(FormatStyle::LanguageKind Language) {
} else {
ChromiumStyle.AllowAllParametersOfDeclarationOnNextLine = false;
ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
+ ChromiumStyle.AllowShortFunctionsOnASingleLineOptions = {};
+ ChromiumStyle.AllowShortFunctionsOnASingleLineOptions.Inline = true;
ChromiumStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
ChromiumStyle.AllowShortLoopsOnASingleLine = false;
ChromiumStyle.BinPackParameters = FormatStyle::BPPS_OnePerLine;
@@ -1907,6 +1954,8 @@ FormatStyle getMozillaStyle() {
FormatStyle MozillaStyle = getLLVMStyle();
MozillaStyle.AllowAllParametersOfDeclarationOnNextLine = false;
MozillaStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
+ MozillaStyle.AllowShortFunctionsOnASingleLineOptions = {};
+ MozillaStyle.AllowShortFunctionsOnASingleLineOptions.Inline = true;
MozillaStyle.AlwaysBreakAfterDefinitionReturnType =
FormatStyle::DRTBS_TopLevel;
MozillaStyle.BinPackArguments = false;
@@ -1989,6 +2038,7 @@ FormatStyle getMicrosoftStyle(FormatStyle::LanguageKind Language) {
Style.PenaltyReturnTypeOnItsOwnLine = 1000;
Style.AllowShortEnumsOnASingleLine = false;
Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_None;
+ Style.AllowShortFunctionsOnASingleLineOptions = {};
Style.AllowShortCaseLabelsOnASingleLine = false;
Style.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
Style.AllowShortLoopsOnASingleLine = false;
@@ -2160,6 +2210,7 @@ std::string configurationAsText(const FormatStyle &Style) {
expandPresetsBraceWrapping(NonConstStyle);
expandPresetsSpaceBeforeParens(NonConstStyle);
expandPresetsSpacesInParens(NonConstStyle);
+ expandPresetsShortFunctionsOnSingleLine(NonConstStyle);
Output << NonConstStyle;
return Stream.str();
@@ -3722,6 +3773,7 @@ reformat(const FormatStyle &Style, StringRef Code,
expandPresetsBraceWrapping(Expanded);
expandPresetsSpaceBeforeParens(Expanded);
expandPresetsSpacesInParens(Expanded);
+ expandPresetsShortFunctionsOnSingleLine(Expanded);
Expanded.InsertBraces = false;
Expanded.RemoveBracesLLVM = false;
Expanded.RemoveParentheses = FormatStyle::RPS_Leave;
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index d87b3a6088bd8..c058354918140 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5687,11 +5687,10 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
if (Right.is(tok::r_brace) && Left.is(tok::l_brace) &&
!Left.Children.empty()) {
// Support AllowShortFunctionsOnASingleLine for JavaScript.
- return Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_None ||
- Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_Empty ||
+ return (!Style.AllowShortFunctionsOnASingleLineOptions.Inline &&
+ !Style.AllowShortFunctionsOnASingleLineOptions.All) ||
(Left.NestingLevel == 0 && Line.Level == 0 &&
- Style.AllowShortFunctionsOnASingleLine &
- FormatStyle::SFS_InlineOnly);
+ Style.AllowShortFunctionsOnASingleLineOptions.Inline);
}
} else if (Style.Language == FormatStyle::LK_Java) {
if (Right.is(tok::plus) && Left.is(tok::string_literal) && AfterRight &&
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 000a5105ca407..5be28e895aab6 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -293,15 +293,14 @@ class LineJoiner {
auto ShouldMergeShortFunctions = [this, &I, &NextLine, PreviousLine,
TheLine]() {
- if (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All)
+ if (Style.AllowShortFunctionsOnASingleLineOptions.All)
return true;
- if (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
+ if (Style.AllowShortFunctionsOnASingleLineOptions.Empty &&
NextLine.First->is(tok::r_brace)) {
return true;
}
- if (Style.AllowShortFunctionsOnASingleLine &
- FormatStyle::SFS_InlineOnly) {
+ if (Style.AllowShortFunctionsOnASingleLineOptions.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))
@@ -551,7 +550,7 @@ class LineJoiner {
unsigned MergedLines = 0;
if (MergeShortFunctions ||
- (Style.AllowShortFunctionsOnASingleLine >= FormatStyle::SFS_Empty &&
+ (Style.AllowShortFunctionsOnASingleLineOptions.Empty &&
NextLine.First == NextLine.Last && I + 2 != E &&
I[2]->First->is(tok::r_brace))) {
MergedLines = tryMergeSimpleBlock(I + 1, E, Limit);
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index 287191d04d885..54e484fcbe4cd 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -256,6 +256,9 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InConditionalStatements);
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InEmptyParentheses);
CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, Other);
+ CHECK_PARSE_NESTED_BOOL(AllowShortFunctionsOnASingleLineOptions, Empty);
+ CHECK_PARSE_NESTED_BOOL(AllowShortFunctionsOnASingleLineOptions, Inline);
+ CHECK_PARSE_NESTED_BOOL(AllowShortFunctionsOnASingleLineOptions, All);
}
#undef CHECK_PARSE_BOOL
@@ -620,6 +623,9 @@ TEST(ConfigParseTest, ParsesConfiguration) {
AllowShortFunctionsOnASingleLine, FormatStyle::SFS_Empty);
CHECK_PARSE("AllowShortFunctionsOnASingleLine: All",
AllowShortFunctionsOnASingleLine, FormatStyle::SFS_All);
+ CHECK_PARSE("AllowShortFunctionsOnASingleLine: Custom",
+ AllowShortFunctionsOnASingleLine, FormatStyle::SFS_Custom);
+
// For backward compatibility:
CHECK_PARSE("AllowShortFunctionsOnASingleLine: false",
AllowShortFunctionsOnASingleLine, FormatStyle::SFS_None);
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 0b90bd360b758..868d4e375b953 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -15120,6 +15120,117 @@ TEST_F(FormatTest, PullInlineFunctionDefinitionsIntoSingleLine) {
MergeInlineOnly);
}
+TEST_F(FormatTest, CustomShortFunctionOptions) {
+ FormatStyle CustomEmpty = getLLVMStyle();
+ CustomEmpty.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Custom;
+ CustomEmpty.AllowShortFunctionsOnASingleLineOptions.Empty = true;
+ CustomEmpty.AllowShortFunctionsOnASingleLineOptions.Inline = false;
+ CustomEmpty.AllowShortFunctionsOnASingleLineOptions.All = false;
+
+ // Empty functions should be on a single line
+ verifyFormat("int f() {}", CustomEmpty);
+ verifyFormat("class C {\n"
+ " int f() {}\n"
+ "};",
+ CustomEmpty);
+
+ // Non-empty functions should be multi-line
+ verifyFormat("int f() {\n"
+ " return 42;\n"
+ "}",
+ CustomEmpty);
+ verifyFormat("class C {\n"
+ " int f() {\n"
+ " return 42;\n"
+ " }\n"
+ "};",
+ CustomEmpty);
+
+ // Test with AfterFunction = true
+ CustomEmpty.BreakBeforeBraces = FormatStyle::BS_Custom;
+ CustomEmpty.BraceWrapping.AfterFunction = true;
+ verifyFormat("int f() {}", CustomEmpty);
+ verifyFormat("int g()\n"
+ "{\n"
+ " return 42;\n"
+ "}",
+ CustomEmpty);
+
+ // Test with Inline = true, All = false
+ FormatStyle CustomInline = getLLVMStyle();
+ CustomInline.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Custom;
+ CustomInline.AllowShortFunctionsOnASingleLineOptions.Empty = false;
+ CustomInline.AllowShortFunctionsOnASingleLineOptions.Inline = true;
+ CustomInline.AllowShortFunctionsOnASingleLineOptions.All = false;
+
+ verifyFormat("class C {\n"
+ " int f() {}\n"
+ "};",
+ CustomInline);
+
+ // Non-empty inline functions should be single-line
+ verifyFormat("class C {\n"
+ " int f() { return 42; }\n"
+ "};",
+ CustomInline);
+
+ // Non-inline functions should be multi-line
+ verifyFormat("int f() {\n"
+ " return 42;\n"
+ "}",
+ CustomInline);
+ verifyFormat("int g() {\n"
+ "}",
+ CustomInline);
+
+ // Test with All = true
+ FormatStyle CustomAll = getLLVMStyle();
+ CustomAll.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Custom;
+ CustomAll.AllowShortFunctionsOnASingleLineOptions.Empty = false;
+ CustomAll.AllowShortFunctionsOnASingleLineOptions.Inline = false;
+ CustomAll.AllowShortFunctionsOnASingleLineOptions.All = true;
+
+ // All functions should be on a single line if they fit
+ verifyFormat("int f() { return 42; }", CustomAll);
+ verifyFormat("int g() { return f() + h(); }", CustomAll);
+ verifyFormat("class C {\n"
+ " int f() { return 42; }\n"
+ "};",
+ CustomAll);
+
+ verifyFormat("int f() {}", CustomAll);
+ verifyFormat("class C {\n"
+ " int f() {}\n"
+ "};",
+ CustomAll);
+...
[truncated]
|
When I changed the |
Reusing the option name is IMO better than adding |
If this was done previously I think this is doable. I will take a look and make appropriate changes. Thanks. |
@@ -1500,7 +1523,10 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) { | |||
LLVMStyle.AllowShortCaseLabelsOnASingleLine = false; | |||
LLVMStyle.AllowShortCompoundRequirementOnASingleLine = true; | |||
LLVMStyle.AllowShortEnumsOnASingleLine = true; | |||
LLVMStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All; | |||
LLVMStyle.AllowShortFunctionsOnASingleLine = | |||
FormatStyle::ShortFunctionStyle({/*Empty=*/true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if it is acceptable to make static functions like FormatStyle::ShortFunctionStyle::SFS_Empty()
(or similar) so we don't have specify each member in constructor every time. This might be useful in the future - I want to add few options to ShortFunctionStyle, and I have to increase number of parameters in constructor. This will require changes in every place where it was used.
AllowShortFunctionsOnASingleLine: | ||
Empty: false | ||
Inline: true | ||
All: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All should probably be other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
clang/lib/Format/TokenAnnotator.cpp
Outdated
Style.AllowShortFunctionsOnASingleLine & | ||
FormatStyle::SFS_InlineOnly); | ||
Style.AllowShortFunctionsOnASingleLine.Inline && | ||
!Style.AllowShortFunctionsOnASingleLine.Other); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you should not add inline here. The old check uses &
. That is same as comparing against SFS_All
. The idea is that the brace should be on a new line if either the AllosShortFunctionsOnASingleLine
is completely disabled, or the function is at the top level and the other option is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the options are now independent, this part should probably become something like this.
!(Left.NestingLevel == 0 && Line.Level == 0 ? Style.AllowShortFunctionsOnASingleLine.Other : Style.AllowShortFunctionsOnASingleLine.Inline)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you should not add inline here. The old check uses
&
. That is same as comparing againstSFS_All
.
See #134337 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the options are now independent, this part should probably become something like this.
!(Left.NestingLevel == 0 && Line.Level == 0 ? Style.AllowShortFunctionsOnASingleLine.Other : Style.AllowShortFunctionsOnASingleLine.Inline)
That seems wrong. See #134337 (comment).
@@ -611,20 +611,40 @@ TEST(ConfigParseTest, ParsesConfiguration) { | |||
CHECK_PARSE("AllowShortBlocksOnASingleLine: true", | |||
AllowShortBlocksOnASingleLine, FormatStyle::SBS_Always); | |||
|
|||
Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline; | |||
// Style.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the comment for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed. Forgot to remove it.
void f() { | ||
} | ||
|
||
* ``bool Other`` Merge all functions fitting on a single line. Please note that this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we expose this option to user? I am not sure what it means if we set only Other, but not Inline and not Empty - then C/C++ code will behave like SFS_All. Probably we should set it when user chooses 'All' in configuration.
Sorry, I forgot to push latest changes |
We should stick to the table in #134337 (comment) and deprecate |
Thanks, I marked options as 'deprecated'. Mapping is done in Format.cpp. |
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp,h -- clang/include/clang/Format/Format.h clang/lib/Format/Format.cpp clang/lib/Format/TokenAnnotator.cpp clang/lib/Format/UnwrappedLineFormatter.cpp clang/unittests/Format/ConfigParseTest.cpp clang/unittests/Format/DefinitionBlockSeparatorTest.cpp clang/unittests/Format/FormatTest.cpp clang/unittests/Format/FormatTestCSharp.cpp clang/unittests/Format/FormatTestJS.cpp clang/unittests/Format/FormatTestJava.cpp View the diff from clang-format here.diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 5e79afe28..7ee954606 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -934,9 +934,7 @@ struct FormatStyle {
ShortFunctionStyle() : Empty(false), Inline(false), Other(false) {}
ShortFunctionStyle(bool Empty, bool Inline, bool Other)
: Empty(Empty), Inline(Inline), Other(Other) {}
- bool isAll() const {
- return Empty && Inline && Other;
- }
+ bool isAll() const { return Empty && Inline && Other; }
};
/// Dependent on the value, ``int f() { return 0; }`` can be put on a
|
607610e
to
5ff5810
Compare
clang/lib/Format/TokenAnnotator.cpp
Outdated
return !(Left.NestingLevel == 0 && Line.Level == 0 | ||
? Style.AllowShortFunctionsOnASingleLine.Other | ||
: Style.AllowShortFunctionsOnASingleLine.Inline); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would return false
for SFS_All
before but returns true
for Other
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I am confused now with these changes.
Previously we had setting SFS_All which means 'everything'. Now we introduced option 'Other' and I cannot create explanation for what it does if user sets only it. (Inline is false, Empty is false, Other is true). The only place it is being used is this code (by used I mean has effect on the result).
I tried to create table to get the difference between old values and new values:
enum |
Custom |
Left.NestingLevel == 0 && Line.Level == 0 | old result | New result |
---|---|---|---|---|
SFS_None |
everything set to false |
true | true | true |
false | true | true | ||
SFS_InlineOnly |
only Inline set to true |
true | true | true |
false | false | false | ||
SFS_Empty |
only Empty set to true |
true | true | true |
false | true | true | ||
SFS_Inline |
both Inline and Empty set to true |
true | true | true |
false | false | false | ||
SFS_All |
everything set to true |
true | false | false |
false | false | false | ||
Other = true | Other = true | true | - | false |
false | - | true |
So new result with only Other==true does not correlate with anything that was before...
Per my understanding, this value (Other) should only be set to true if SFS_All option was specified. In other cases 'Other' does not make sense. I think we should not allow user to set it directly (only through SFS_All). Please let me know what do you think. In this case code can be simplified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new struct is a partition of all cases, with other
being "everything else", i.e., any cases not covered by Empty
(or SFS_Empty
before), Inline
(or SFS_InlineOnly
before), and any future struct members. This is similar to Other
of SpacesInParensOptions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only Other
is set, we should get the following format:
void topLevelEmpty() {
}
void topLevelNonEmpty() { return; }
struct Foo {
void inlineEmpty() {
}
void inlineNonEmpty() {
return;
}
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explanation! I think I finally understood what Other mean.
I've added your example to unit test.
@@ -293,15 +293,14 @@ class LineJoiner { | |||
|
|||
auto ShouldMergeShortFunctions = [this, &I, &NextLine, PreviousLine, | |||
TheLine]() { | |||
if (Style.AllowShortFunctionsOnASingleLine == FormatStyle::SFS_All) | |||
if (Style.AllowShortFunctionsOnASingleLine.Other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't SFS_All
equivalent to Empty && Inline && Other
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
clang/lib/Format/TokenAnnotator.cpp
Outdated
Style.AllowShortFunctionsOnASingleLine & | ||
FormatStyle::SFS_InlineOnly); | ||
Style.AllowShortFunctionsOnASingleLine.Inline && | ||
!Style.AllowShortFunctionsOnASingleLine.Other); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you should not add inline here. The old check uses
&
. That is same as comparing againstSFS_All
.
See #134337 (comment).
NextLine.First->is(tok::r_brace)) { | ||
return true; | ||
} | ||
|
||
if (Style.AllowShortFunctionsOnASingleLine & | ||
FormatStyle::SFS_InlineOnly) { | ||
if (Style.AllowShortFunctionsOnASingleLine.Inline) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #134337 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think because we have check for All in the beginning, we don't need to add another check !All here as it always be true.
@irymarchyk can you resolve the conflicts? |
…FunctionsOnASingleLine
Thanks, I hadn't noticed that. Now PR does not have conflicts. |
The current clang-format configuration option AllowShortFunctionsOnASingleLine uses a single enum (ShortFunctionStyle) to control when short function definitions can be merged onto a single line. This enum provides predefined combinations of conditions (e.g., None, Empty only, Inline only, Inline including Empty, All).
This approach has limitations:
Lack of Granularity: Users cannot specify arbitrary combinations of conditions. For example, a user might want to allow merging for both empty functions and short top-level functions, but not for short functions defined within classes. This is not possible with the current enum options except by choosing All, which might merge more than desired.
Inflexibility: Adding new conditions for merging (e.g., distinguishing between member functions and constructors, handling lambdas specifically) would require adding many new combined enum values, leading to a combinatorial explosion and making the configuration complex.
Implicit Behavior: Some options imply others (e.g., Inline implies Empty), which might not always be intuitive or desired.
The goal is to replace this single-choice enum with a more flexible mechanism allowing users to specify a set of conditions that must be met for a short function to be merged onto a single line.
Proposed Solution
Introduce a new configuration option: AllowShortFunctionsOnSingleLineOptions.
This option will accept a list of strings, where each string represents a specific condition allowing merging.
Backward Compatibility:
If AllowShortFunctionsOnSingleLineOptions is present in the configuration, it takes precedence.
If AllowShortFunctionsOnSingleLineOptions is not present, but the old AllowShortFunctionsOnASingleLine is present, the old option should be parsed and mapped to the corresponding new semantics for compatibility.