[clang-format] Change BinPackParameters to enum and add AlwaysOnePerLine #101882
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be 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 If you have received no comments on your PR for a week, you can request a review 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. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-format @llvm/pr-subscribers-clang Author: None (VolatileAcorn) ChangesDeprecated the BinPackParameters boolean option in favor or PackParameters enum option in order to provide a BreakAlways setting. Related issues that have requested this feature Patch is 32.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/101882.diff 10 Files Affected:
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 6c2e6da594847..510b956968dc6 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -2068,19 +2068,7 @@ the configuration (without a prefix: ``Auto``).
.. _BinPackParameters:
**BinPackParameters** (``Boolean``) :versionbadge:`clang-format 3.7` :ref:`¶ <BinPackParameters>`
- If ``false``, a function declaration's or function definition's
- parameters will either all be on the same line or will have one line each.
-
- .. code-block:: c++
-
- true:
- void f(int aaaaaaaaaaaaaaaaaaaa, int aaaaaaaaaaaaaaaaaaaa,
- int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
-
- false:
- void f(int aaaaaaaaaaaaaaaaaaaa,
- int aaaaaaaaaaaaaaaaaaaa,
- int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
+ This option is **deprecated**. See ``PackParameters``.
.. _BitFieldColonSpacing:
@@ -4984,6 +4972,44 @@ the configuration (without a prefix: ``Auto``).
+.. _PackParameters:
+
+**PackParameters** (``PackParametersStyle``) :versionbadge:`clang-format 20` :ref:`¶ <PackParameters>`
+ The pack parameters style to use.
+
+ Possible values:
+
+ * ``PPS_CurrentLine`` (in configuration: ``CurrentLine``)
+ Put all parameters on the current line if they fit.
+ Otherwise, put each one on its own line.
+
+ .. code-block:: c++
+
+ void f(int a, int b, int c);
+
+ void f(int a,
+ int b,
+ int ccccccccccccccccccccccccccccccccccccc);
+
+ * ``PPS_BinPack`` (in configuration: ``BinPack``)
+ Bin-pack parameters.
+
+ .. code-block:: c++
+
+ void f(int a, int bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
+ int ccccccccccccccccccccccccccccccccccccccccccc);
+
+ * ``PPS_BreakAlways`` (in configuration: ``BreakAlways``)
+ Always put each parameter on its own line.
+
+ .. code-block:: c++
+
+ void f(int a,
+ int b,
+ int c);
+
+
+
.. _PenaltyBreakAssignment:
**PenaltyBreakAssignment** (``Unsigned``) :versionbadge:`clang-format 5` :ref:`¶ <PenaltyBreakAssignment>`
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index c454ab2bc0ce2..2991ab18f980d 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -1192,20 +1192,9 @@ struct FormatStyle {
/// \version 3.7
bool BinPackArguments;
- /// If ``false``, a function declaration's or function definition's
- /// parameters will either all be on the same line or will have one line each.
- /// \code
- /// true:
- /// void f(int aaaaaaaaaaaaaaaaaaaa, int aaaaaaaaaaaaaaaaaaaa,
- /// int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
- ///
- /// false:
- /// void f(int aaaaaaaaaaaaaaaaaaaa,
- /// int aaaaaaaaaaaaaaaaaaaa,
- /// int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {}
- /// \endcode
+ /// This option is **deprecated**. See ``PackParameters``.
/// \version 3.7
- bool BinPackParameters;
+ // bool BinPackParameters;
/// Styles for adding spacing around ``:`` in bitfield definitions.
enum BitFieldColonSpacingStyle : int8_t {
@@ -3537,6 +3526,37 @@ struct FormatStyle {
/// \version 14
PackConstructorInitializersStyle PackConstructorInitializers;
+ /// Different way to try to fit all parameters on a line.
+ enum PackParametersStyle : int8_t {
+ /// Put all parameters on the current line if they fit.
+ /// Otherwise, put each one on its own line.
+ /// \code
+ /// void f(int a, int b, int c);
+ ///
+ /// void f(int a,
+ /// int b,
+ /// int ccccccccccccccccccccccccccccccccccccc);
+ /// \endcode
+ PPS_CurrentLine,
+ /// Bin-pack parameters.
+ /// \code
+ /// void f(int a, int bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb,
+ /// int ccccccccccccccccccccccccccccccccccccccccccc);
+ /// \endcode
+ PPS_BinPack,
+ /// Always put each parameter on its own line.
+ /// \code
+ /// void f(int a,
+ /// int b,
+ /// int c);
+ /// \endcode
+ PPS_BreakAlways,
+ };
+
+ /// The pack parameters style to use.
+ /// \version 20
+ PackParametersStyle PackParameters;
+
/// The penalty for breaking around an assignment operator.
/// \version 5
unsigned PenaltyBreakAssignment;
@@ -5024,7 +5044,6 @@ struct FormatStyle {
R.AlwaysBreakBeforeMultilineStrings &&
AttributeMacros == R.AttributeMacros &&
BinPackArguments == R.BinPackArguments &&
- BinPackParameters == R.BinPackParameters &&
BitFieldColonSpacing == R.BitFieldColonSpacing &&
BracedInitializerIndentWidth == R.BracedInitializerIndentWidth &&
BreakAdjacentStringLiterals == R.BreakAdjacentStringLiterals &&
@@ -5094,6 +5113,7 @@ struct FormatStyle {
ObjCSpaceAfterProperty == R.ObjCSpaceAfterProperty &&
ObjCSpaceBeforeProtocolList == R.ObjCSpaceBeforeProtocolList &&
PackConstructorInitializers == R.PackConstructorInitializers &&
+ PackParameters == R.PackParameters &&
PenaltyBreakAssignment == R.PenaltyBreakAssignment &&
PenaltyBreakBeforeFirstCallParameter ==
R.PenaltyBreakBeforeFirstCallParameter &&
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index df86a774ba0f4..8495c7a7314e2 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -128,24 +128,6 @@ static bool startsSegmentOfBuilderTypeCall(const FormatToken &Tok) {
return Tok.isMemberAccess() && Tok.Previous && Tok.Previous->closesScope();
}
-// Returns \c true if \c Current starts a new parameter.
-static bool startsNextParameter(const FormatToken &Current,
- const FormatStyle &Style) {
- const FormatToken &Previous = *Current.Previous;
- if (Current.is(TT_CtorInitializerComma) &&
- Style.BreakConstructorInitializers == FormatStyle::BCIS_BeforeComma) {
- return true;
- }
- if (Style.Language == FormatStyle::LK_Proto && Current.is(TT_SelectorName))
- return true;
- return Previous.is(tok::comma) && !Current.isTrailingComment() &&
- ((Previous.isNot(TT_CtorInitializerComma) ||
- Style.BreakConstructorInitializers !=
- FormatStyle::BCIS_BeforeComma) &&
- (Previous.isNot(TT_InheritanceComma) ||
- Style.BreakInheritanceList != FormatStyle::BILS_BeforeComma));
-}
-
static bool opensProtoMessageField(const FormatToken &LessTok,
const FormatStyle &Style) {
if (LessTok.isNot(tok::less))
@@ -411,7 +393,8 @@ bool ContinuationIndenter::mustBreak(const LineState &State) {
// sets BreakBeforeParameter to avoid bin packing and this creates a
// completely unnecessary line break after a template type that isn't
// line-wrapped.
- (Previous.NestingLevel == 1 || Style.BinPackParameters)) ||
+ (Previous.NestingLevel == 1 ||
+ (Style.PackParameters == FormatStyle::PPS_BinPack))) ||
(Style.BreakBeforeTernaryOperators && Current.is(TT_ConditionalExpr) &&
Previous.isNot(tok::question)) ||
(!Style.BreakBeforeTernaryOperators &&
@@ -1918,11 +1901,12 @@ void ContinuationIndenter::moveStatePastScopeOpener(LineState &State,
// for backwards compatibility.
bool ObjCBinPackProtocolList =
(Style.ObjCBinPackProtocolList == FormatStyle::BPS_Auto &&
- Style.BinPackParameters) ||
+ (Style.PackParameters == FormatStyle::PPS_BinPack)) ||
Style.ObjCBinPackProtocolList == FormatStyle::BPS_Always;
bool BinPackDeclaration =
- (State.Line->Type != LT_ObjCDecl && Style.BinPackParameters) ||
+ (State.Line->Type != LT_ObjCDecl &&
+ (Style.PackParameters == FormatStyle::PPS_BinPack)) ||
(State.Line->Type == LT_ObjCDecl && ObjCBinPackProtocolList);
bool GenericSelection =
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 7fd42e46e0ccb..96b9c50f3068e 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -464,6 +464,14 @@ struct ScalarEnumerationTraits<FormatStyle::PackConstructorInitializersStyle> {
}
};
+template <> struct ScalarEnumerationTraits<FormatStyle::PackParametersStyle> {
+ static void enumeration(IO &IO, FormatStyle::PackParametersStyle &Value) {
+ IO.enumCase(Value, "CurrentLine", FormatStyle::PPS_CurrentLine);
+ IO.enumCase(Value, "BinPack", FormatStyle::PPS_BinPack);
+ IO.enumCase(Value, "BreakAlways", FormatStyle::PPS_BreakAlways);
+ }
+};
+
template <> struct ScalarEnumerationTraits<FormatStyle::PointerAlignmentStyle> {
static void enumeration(IO &IO, FormatStyle::PointerAlignmentStyle &Value) {
IO.enumCase(Value, "Middle", FormatStyle::PAS_Middle);
@@ -852,6 +860,15 @@ template <> struct MappingTraits<FormatStyle> {
bool OnCurrentLine = IsGoogleOrChromium;
bool OnNextLine = true;
+ // For backward compatibility:
+ // The default value of BinPackParameters was true unless BasedOnStyle was
+ // Mozilla or Chromium. If BinPackParameters was true then the equilvalent
+ // value for PackParameters is PPS_BinPack and PPS_CurrentLine otherwise.
+ const bool IsChromiumOrMozilla =
+ BasedOnStyle.equals_insensitive("chromium") ||
+ BasedOnStyle.equals_insensitive("mozilla");
+ bool BinPackParameters = !IsChromiumOrMozilla;
+
bool BreakBeforeInheritanceComma = false;
bool BreakConstructorInitializersBeforeComma = false;
@@ -870,6 +887,7 @@ template <> struct MappingTraits<FormatStyle> {
IO.mapOptional("AlwaysBreakAfterReturnType", Style.BreakAfterReturnType);
IO.mapOptional("AlwaysBreakTemplateDeclarations",
Style.BreakTemplateDeclarations);
+ IO.mapOptional("BinPackParameters", BinPackParameters);
IO.mapOptional("BreakBeforeInheritanceComma",
BreakBeforeInheritanceComma);
IO.mapOptional("BreakConstructorInitializersBeforeComma",
@@ -947,7 +965,6 @@ template <> struct MappingTraits<FormatStyle> {
Style.AlwaysBreakBeforeMultilineStrings);
IO.mapOptional("AttributeMacros", Style.AttributeMacros);
IO.mapOptional("BinPackArguments", Style.BinPackArguments);
- IO.mapOptional("BinPackParameters", Style.BinPackParameters);
IO.mapOptional("BitFieldColonSpacing", Style.BitFieldColonSpacing);
IO.mapOptional("BracedInitializerIndentWidth",
Style.BracedInitializerIndentWidth);
@@ -1037,6 +1054,7 @@ template <> struct MappingTraits<FormatStyle> {
Style.ObjCSpaceBeforeProtocolList);
IO.mapOptional("PackConstructorInitializers",
Style.PackConstructorInitializers);
+ IO.mapOptional("PackParameters", Style.PackParameters);
IO.mapOptional("PenaltyBreakAssignment", Style.PenaltyBreakAssignment);
IO.mapOptional("PenaltyBreakBeforeFirstCallParameter",
Style.PenaltyBreakBeforeFirstCallParameter);
@@ -1174,6 +1192,18 @@ template <> struct MappingTraits<FormatStyle> {
Style.PackConstructorInitializers = FormatStyle::PCIS_CurrentLine;
}
+ // If BinPackParameters was specified but PackParameters was not, initialize
+ // the latter from the former for backwards compatibility.
+ if (IsChromiumOrMozilla) {
+ if (BinPackParameters &&
+ (Style.PackParameters == FormatStyle::PPS_CurrentLine)) {
+ Style.PackParameters = FormatStyle::PPS_BinPack;
+ }
+ } else if (!BinPackParameters &&
+ (Style.PackParameters == FormatStyle::PPS_BinPack)) {
+ Style.PackParameters = FormatStyle::PPS_CurrentLine;
+ }
+
if (Style.LineEnding == FormatStyle::LE_DeriveLF) {
if (!DeriveLineEnding)
Style.LineEnding = UseCRLF ? FormatStyle::LE_CRLF : FormatStyle::LE_LF;
@@ -1449,7 +1479,6 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
LLVMStyle.AttributeMacros.push_back("__capability");
LLVMStyle.BinPackArguments = true;
- LLVMStyle.BinPackParameters = true;
LLVMStyle.BitFieldColonSpacing = FormatStyle::BFCS_Both;
LLVMStyle.BracedInitializerIndentWidth = std::nullopt;
LLVMStyle.BraceWrapping = {/*AfterCaseLabel=*/false,
@@ -1543,6 +1572,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
LLVMStyle.ObjCSpaceAfterProperty = false;
LLVMStyle.ObjCSpaceBeforeProtocolList = true;
LLVMStyle.PackConstructorInitializers = FormatStyle::PCIS_BinPack;
+ LLVMStyle.PackParameters = FormatStyle::PPS_BinPack;
LLVMStyle.PointerAlignment = FormatStyle::PAS_Right;
LLVMStyle.PPIndentWidth = -1;
LLVMStyle.QualifierAlignment = FormatStyle::QAS_Leave;
@@ -1823,8 +1853,8 @@ FormatStyle getChromiumStyle(FormatStyle::LanguageKind Language) {
ChromiumStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Inline;
ChromiumStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
ChromiumStyle.AllowShortLoopsOnASingleLine = false;
- ChromiumStyle.BinPackParameters = false;
ChromiumStyle.DerivePointerAlignment = false;
+ ChromiumStyle.PackParameters = FormatStyle::PPS_CurrentLine;
if (Language == FormatStyle::LK_ObjC)
ChromiumStyle.ColumnLimit = 80;
}
@@ -1838,7 +1868,6 @@ FormatStyle getMozillaStyle() {
MozillaStyle.AlwaysBreakAfterDefinitionReturnType =
FormatStyle::DRTBS_TopLevel;
MozillaStyle.BinPackArguments = false;
- MozillaStyle.BinPackParameters = false;
MozillaStyle.BreakAfterReturnType = FormatStyle::RTBS_TopLevel;
MozillaStyle.BreakBeforeBraces = FormatStyle::BS_Mozilla;
MozillaStyle.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
@@ -1851,6 +1880,7 @@ FormatStyle getMozillaStyle() {
MozillaStyle.IndentCaseLabels = true;
MozillaStyle.ObjCSpaceAfterProperty = true;
MozillaStyle.ObjCSpaceBeforeProtocolList = false;
+ MozillaStyle.PackParameters = FormatStyle::PPS_CurrentLine;
MozillaStyle.PenaltyReturnTypeOnItsOwnLine = 200;
MozillaStyle.PointerAlignment = FormatStyle::PAS_Left;
MozillaStyle.SpaceAfterTemplateKeyword = false;
diff --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index abcedb66b57cc..fb97f4630ab24 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -1978,6 +1978,24 @@ inline bool continuesLineComment(const FormatToken &FormatTok,
FormatTok.OriginalColumn >= MinContinueColumn;
}
+// Returns \c true if \c Current starts a new parameter.
+inline bool startsNextParameter(const FormatToken &Current,
+ const FormatStyle &Style) {
+ const FormatToken &Previous = *Current.Previous;
+ if (Current.is(TT_CtorInitializerComma) &&
+ Style.BreakConstructorInitializers == FormatStyle::BCIS_BeforeComma) {
+ return true;
+ }
+ if (Style.Language == FormatStyle::LK_Proto && Current.is(TT_SelectorName))
+ return true;
+ return Previous.is(tok::comma) && !Current.isTrailingComment() &&
+ ((Previous.isNot(TT_CtorInitializerComma) ||
+ Style.BreakConstructorInitializers !=
+ FormatStyle::BCIS_BeforeComma) &&
+ (Previous.isNot(TT_InheritanceComma) ||
+ Style.BreakInheritanceList != FormatStyle::BILS_BeforeComma));
+}
+
} // namespace format
} // namespace clang
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 4ed3e9d0e8e85..99a137cd6477d 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -5458,6 +5458,14 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line,
return true;
}
+ // Ignores the first parameter as this will be handled separately by
+ // BreakFunctionDefinitionParameters or AlignAfterOpenBracket.
+ if ((FormatStyle::PPS_BreakAlways == Style.PackParameters) &&
+ Line.MightBeFunctionDecl && !Left.opensScope() &&
+ startsNextParameter(Right, Style)) {
+ return true;
+ }
+
const auto *BeforeLeft = Left.Previous;
const auto *AfterRight = Right.Next;
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index cc044153b7c9b..20d5d9c24b32f 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -160,7 +160,6 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
CHECK_PARSE_BOOL(BinPackArguments);
- CHECK_PARSE_BOOL(BinPackParameters);
CHECK_PARSE_BOOL(BreakAdjacentStringLiterals);
CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
CHECK_PARSE_BOOL(BreakBeforeTernaryOperators);
@@ -456,6 +455,21 @@ TEST(ConfigParseTest, ParsesConfiguration) {
"AllowAllConstructorInitializersOnNextLine: false",
PackConstructorInitializers, FormatStyle::PCIS_CurrentLine);
+ Style.PackParameters = FormatStyle::PPS_BinPack;
+ CHECK_PARSE("PackParameters: CurrentLine", PackParameters,
+ FormatStyle::PPS_CurrentLine);
+ CHECK_PARSE("PackParameters: BinPack", PackParameters,
+ FormatStyle::PPS_BinPack);
+ CHECK_PARSE("PackParameters: BreakAlways", PackParameters,
+ FormatStyle::PPS_BreakAlways);
+ // For backward compatibility
+ CHECK_PARSE("BasedOnStyle: Mozilla\n"
+ "BinPackParameters: true",
+ PackParameters, FormatStyle::PPS_BinPack);
+ CHECK_PARSE("BasedOnStyle: Llvm\n"
+ "BinPackParameters: false",
+ PackParameters, FormatStyle::PPS_CurrentLine);
+
Style.EmptyLineBeforeAccessModifier = FormatStyle::ELBAMS_LogicalBlock;
CHECK_PARSE("EmptyLineBeforeAccessModifier: Never",
EmptyLineBeforeAccessModifier, FormatStyle::ELBAMS_Never);
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 2a754a29e81e7..6de132ee4af17 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -2338,7 +2338,7 @@ TEST_F(FormatTest, FormatsForLoop) {
"for (const Foo<Bar> &baz = in.value(); !baz.at_end(); ++baz) {\n}");
FormatStyle NoBinPacking = getLLVMStyle();
- NoBinPacking.BinPackParameters = false;
+ NoBinPacking.PackParameters = FormatStyle::PPS_CurrentLine;
verifyFormat("for (int aaaaaaaaaaa = 1;\n"
" aaaaaaaaaaa <= aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaa,\n"
" aaaaaaaaaaaaaaaa,\n"
@@ -7165,7 +7165,7 @@ TEST_F(FormatTest, LineBreakingInBinaryExpressions) {
"}");
FormatStyle OnePerLine = getLLVMStyle();
- OnePerLine.BinPackParameters = false;
+ OnePerLine.PackParameters = FormatStyle::PPS_CurrentLine;
verifyFormat(
"if (aaaaaaaaaaaaaaaaaaaaaaaaaaaa || aaaaaaaaaaaaaaaaaaaaaaaaaaaa ||\n"
" aaaaaaaaaaaaaaaaaaaaaaaaaaaa || aaaaaaaaaaaaaaaaaaaaaaaaaaaa ||\n"
@@ -7319,7 +7319,7 @@ TEST_F(FormatTest, ExpressionIndentationBreakingBeforeOperators) {
Style = getLLVMStyleWithColumns(20);
Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
- Style.BinPackParameters = false;
+ Style.PackParameters = FormatStyle::PPS_CurrentLine;
Style.BreakBeforeBinaryOperators = FormatStyle::BOS_NonAssignment;
Style.ContinuationIndentWidth = 2;
verifyFormat("struct Foo {\n"
@@ -7694,7 +7694,7 @@ TEST_F(FormatTest, ConstructorInitializers) {
" : aaaaa(aaaaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaa,\n"
" aaaaaaaaaaaaaaaaaaaaaa) {}",
OnePerLine);
- OnePerLine.BinPackParameters = false;
+ OnePerLine.PackParameters = FormatStyle::PPS_CurrentLine;
verifyFormat(
"Constructor()\n"
" : aaaaaaaaaaaaaaaaaaaaaaaa(\n"
@@ -7718,7 +7718,7 @@ TEST_F(FormatTest, ConstructorInitializers) {
TEST_F(FormatTest, AllowAllConstructorInitializersOnNextLine) {
FormatStyle Style = getLLVMStyleWithColumns(60);
Style.BreakConstructorInitializers = FormatStyle::BCIS_BeforeComma;
- Style.BinPackParameters = false;
+ Style.PackParameters = FormatStyle::PPS_CurrentLine;
for (int i = 0; i < 4; ++i) {
...
[truncated]
|
VolatileAcorn
force-pushed
the
Feature/PackArgParamOption
branch
from
6739bb5 to
923a4ef
Compare
|
Sorry, I incorrectly assumed that I needed to create a new option if I was changing an option from a bool to an enum. |
VolatileAcorn
force-pushed
the
Feature/PackArgParamOption
branch
from
923a4ef to
1dda629
Compare
VolatileAcorn
changed the title
There was a problem hiding this comment.
In principle I think this is ok, but I'd like to hear @owenca and @HazardyKnusperkeks opinion
|
We should rename the option and its values to e.g. |
|
When you say rename, do you mean deprecate |
VolatileAcorn
force-pushed
the
Feature/PackArgParamOption
branch
from
1dda629 to
e6f04cd
Compare
Yes, deprecate Have you given any thought about #53190 (comment)? |
|
Whilst I can understand BinPackParameters being deprecated, as its one of the original options I do worry a little about just how much impact such a deprecation might have... it needs to be completely seemless because there is ALOT of documentation references to its use on say stackoverflow. etc... One reason I suggested making the change without renaming the option was the level of flux the name change was having. I couldn't really see in the review what was a change for the name/enum and what was the new behaviour. (maybe that could be done in 2 separate commits?) |
I couldn't see a simple way to implement Leave, but it's possible I've missed something.
If BinPackArguments was also to be converted to an enum then yes I think it would make sense to update BAS_AlwaysBreak and BAS_BlockIndent, but if it is done now then I think it could be seen as inconsistent / confusing. |
Since BinPackParameters: false / true can still be used with either approach, I assume the main worry would be that some stack overflow answer about an odd behavior / interaction of BinPackParameters that BreakParameters inherits will be less easy to find for people using the new option?
I'd certainly be happy to split the changes into 2 commits, if this is the approach you all want to go with. |
I had never reviewed a similar PR (until #96804) let alone having worked on one. Now I understand it's better to keep the option name even if it's confusing. |
My bad! @mydeveloperday was right. So let's keep the name |
VolatileAcorn
force-pushed
the
Feature/PackArgParamOption
branch
from
e6f04cd to
ddb5bad
Compare
No problem, I've gone back to the previous changes but updated names of the values and rebased to fix the conflict with the startsNextParameter function |
|
There's a compiler warning: |
clang/lib/Format/TokenAnnotator.cpp
Outdated
| @@ -5475,6 +5475,14 @@ bool TokenAnnotator::mustBreakBefore(const AnnotatedLine &Line, | |||
| return true; | |||
| } | |||
|
|
|||
| // Ignores the first parameter as this will be handled separately by | |||
| // BreakFunctionDefinitionParameters or AlignAfterOpenBracket. | |||
| if (FormatStyle::BPPS_OnePerLine == Style.BinPackParameters && | |||
There was a problem hiding this comment.
| if (FormatStyle::BPPS_OnePerLine == Style.BinPackParameters && | |
| if (Style.BinPackParameters == FormatStyle::BPPS_Never && |
Actually, we renamed boolean options before when extending them to |
…ated tests to actually test parameters and not arguments.
owenca
changed the title
|
@VolatileAcorn Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…ine (llvm#101882) Related issues that have requested this feature: llvm#51833 llvm#23796 llvm#53190 Partially solves - this issue requests is for both arguments and parameters
Related issues that have requested this feature:
#51833
#23796
#53190 Partially solves - this issue requests is for both arguments and parameters