[clang-format] Add BreakBinaryOperations configuration #95013

ameerj · 2024-06-10T17:27:52Z

By default, clang-format packs binary operations, but it may be desirable to have compound operations be on individual lines instead of being packed.

This PR adds the option BreakBinaryOperations to break up large compound binary operations to be on one line each.

This applies to all logical and arithmetic/bitwise binary operations

Maybe partially addresses #79487 ?
Closes #58014
Closes #57280

llvmbot · 2024-06-10T17:28:24Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Ameer J (ameerj)

Changes

By default, clang-format packs binary operations, but it may be desirable to have compound operations be on individual lines instead of being packed.

This PR adds the option BinPackBinaryOperations, which mimics the behavior of BinPackArguments and BinPackParameters, but applied to binary operations.
When BinPackBinaryOperations is set to false, binary operations will either be all on the same line, or each operation will have one line each.

This applies to all logical and arithmetic/bitwise binary operations

Maybe partially addresses #79487 ?

Full diff: https://github.com/llvm/llvm-project/pull/95013.diff

5 Files Affected:

(modified) clang/docs/ClangFormatStyleOptions.rst (+17)
(modified) clang/include/clang/Format/Format.h (+16)
(modified) clang/lib/Format/ContinuationIndenter.cpp (+15)
(modified) clang/lib/Format/Format.cpp (+2)
(modified) clang/unittests/Format/FormatTest.cpp (+80)

diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index bb00c20922d36..246d98eef63e8 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -2065,6 +2065,23 @@ the configuration (without a prefix: ``Auto``).
         aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);
     }
 
+.. _BinPackBinaryOperations:
+
+**BinPackBinaryOperations** (``Boolean``) :versionbadge:`clang-format 19` :ref:`¶ <BinPackBinaryOperations>`
+  If ``false``, binary operations will either be all on the same line
+  or each operation will have one line each.
+
+  .. code-block:: c++
+
+    true:
+    aaaaaaaaaaaaaaaaaaaa && aaaaaaaaaaaaaaaaaaaa ||
+    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;
+
+    false:
+    aaaaaaaaaaaaaaaaaaaa &&
+    aaaaaaaaaaaaaaaaaaaa ||
+    aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;
+
 .. _BinPackParameters:
 
 **BinPackParameters** (``Boolean``) :versionbadge:`clang-format 3.7` :ref:`¶ <BinPackParameters>`
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 4fd6e013df25b..19b9c60ab6e99 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -1192,6 +1192,21 @@ struct FormatStyle {
   /// \version 3.7
   bool BinPackArguments;
 
+  /// If ``false``, binary operations will either be all on the same line
+  /// or each operation will have one line each.
+  /// \code
+  ///   true:
+  ///   aaaaaaaaaaaaaaaaaaaa && aaaaaaaaaaaaaaaaaaaa ||
+  ///   aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;
+  ///
+  ///   false:
+  ///   aaaaaaaaaaaaaaaaaaaa &&
+  ///   aaaaaaaaaaaaaaaaaaaa ||
+  ///   aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa;
+  /// \endcode
+  /// \version 19
+  bool BinPackBinaryOperations;
+
   /// 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
@@ -4978,6 +4993,7 @@ struct FormatStyle {
                R.AlwaysBreakBeforeMultilineStrings &&
            AttributeMacros == R.AttributeMacros &&
            BinPackArguments == R.BinPackArguments &&
+           BinPackBinaryOperations == R.BinPackBinaryOperations &&
            BinPackParameters == R.BinPackParameters &&
            BitFieldColonSpacing == R.BitFieldColonSpacing &&
            BracedInitializerIndentWidth == R.BracedInitializerIndentWidth &&
diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp
index b07360425ca6e..0780d219b68b4 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -146,6 +146,14 @@ static bool startsNextParameter(const FormatToken &Current,
            Style.BreakInheritanceList != FormatStyle::BILS_BeforeComma));
 }
 
+// Returns \c true if \c Current starts a new operand in a binary operation.
+static bool startsNextOperand(const FormatToken &Current,
+                              const FormatStyle &Style) {
+  const FormatToken &Previous = *Current.Previous;
+  return Previous.is(TT_BinaryOperator) && !Current.isTrailingComment() &&
+         (Previous.getPrecedence() > prec::Conditional);
+}
+
 static bool opensProtoMessageField(const FormatToken &LessTok,
                                    const FormatStyle &Style) {
   if (LessTok.isNot(tok::less))
@@ -837,6 +845,9 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState &State, bool DryRun,
   }
   if (CurrentState.AvoidBinPacking && startsNextParameter(Current, Style))
     CurrentState.NoLineBreak = true;
+  if (!Style.BinPackBinaryOperations && startsNextOperand(Current, Style))
+    CurrentState.NoLineBreak = true;
+
   if (startsSegmentOfBuilderTypeCall(Current) &&
       State.Column > getNewLineColumn(State)) {
     CurrentState.ContainsUnwrappedBuilder = true;
@@ -1203,6 +1214,10 @@ unsigned ContinuationIndenter::addTokenOnNewLine(LineState &State,
     }
   }
 
+  if (!Style.BinPackBinaryOperations && Previous.is(TT_BinaryOperator) &&
+      (Previous.getPrecedence() > prec::Conditional)) {
+    CurrentState.BreakBeforeParameter = true;
+  }
   return Penalty;
 }
 
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index c015e03fa15e7..4d71ce95afe1d 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -935,6 +935,7 @@ template <> struct MappingTraits<FormatStyle> {
                    Style.AlwaysBreakBeforeMultilineStrings);
     IO.mapOptional("AttributeMacros", Style.AttributeMacros);
     IO.mapOptional("BinPackArguments", Style.BinPackArguments);
+    IO.mapOptional("BinPackBinaryOperations", Style.BinPackBinaryOperations);
     IO.mapOptional("BinPackParameters", Style.BinPackParameters);
     IO.mapOptional("BitFieldColonSpacing", Style.BitFieldColonSpacing);
     IO.mapOptional("BracedInitializerIndentWidth",
@@ -1439,6 +1440,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
   LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
   LLVMStyle.AttributeMacros.push_back("__capability");
   LLVMStyle.BinPackArguments = true;
+  LLVMStyle.BinPackBinaryOperations = true;
   LLVMStyle.BinPackParameters = true;
   LLVMStyle.BitFieldColonSpacing = FormatStyle::BFCS_Both;
   LLVMStyle.BracedInitializerIndentWidth = std::nullopt;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index fb57333858529..cb20c5db4884b 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -27492,6 +27492,86 @@ TEST_F(FormatTest, SpaceBetweenKeywordAndLiteral) {
   verifyFormat("return sizeof \"5\";");
 }
 
+TEST_F(FormatTest, BinPackBinaryOperations) {
+  auto Style = getLLVMStyle();
+  Style.BinPackBinaryOperations = false;
+
+  // Logical operations
+  verifyFormat("if (condition1 && condition2) {\n"
+               "}",
+               Style);
+  verifyFormat("if (condition1 && // comment\n"
+               "    condition2 &&\n"
+               "    (condition3 || condition4) && // comment\n"
+               "    condition5 &&\n"
+               "    condition6) {\n"
+               "}",
+               Style);
+  verifyFormat("if (loooooooooooooooooooooongcondition1 &&\n"
+               "    loooooooooooooooooooooongcondition2) {\n"
+               "}",
+               Style);
+
+  // Arithmetic
+  verifyFormat("const int result = lhs + rhs;\n", Style);
+  verifyFormat("const int result = loooooooooooooooooooooongop1 +\n"
+               "                   loooooooooooooooooooooongop2 +\n"
+               "                   loooooooooooooooooooooongop3;\n",
+               Style);
+
+  // clang-format off
+  verifyFormat("const int result =\n"
+               "    operand1 + operand2 - (operand3 + operand4) - operand5 * operand6;\n",
+               Style);
+  // clang-format on
+
+  verifyFormat("const int result = longOperand1 +\n"
+               "                   longOperand2 -\n"
+               "                   (longOperand3 + longOperand4) -\n"
+               "                   longOperand5 * longOperand6;\n",
+               Style);
+
+  Style.BinPackBinaryOperations = true;
+
+  // Logical operations
+  verifyFormat("if (condition1 && condition2) {\n"
+               "}",
+               Style);
+
+  // clang-format off
+  verifyFormat("if (condition1 && condition2 && (condition3 || condition4) && condition5 &&\n"
+               "    condition6) {\n"
+               "}",
+               Style);
+  // clang-format on
+
+  verifyFormat("if (loooooooooooooooooooooongcondition1 &&\n"
+               "    loooooooooooooooooooooongcondition2) {\n"
+               "}",
+               Style);
+
+  // Arithmetic
+  verifyFormat("const int result = lhs + rhs;\n", Style);
+
+  // clang-format off
+  verifyFormat("const int result = loooooooooooooooooooooongop1 + loooooooooooooooooooooongop2 +\n"
+               "                   loooooooooooooooooooooongop3;\n",
+               Style);
+  // clang-format on
+
+  // clang-format off
+  verifyFormat("const int result = longOperand1 + longOperand2 - (longOperand3 + longOperand4) -\n"
+               "                   longOperand5 * longOperand6;\n",
+               Style);
+  // clang-format on
+
+  // clang-format off
+  verifyFormat("const int result =\n"
+               "    operand1 + operand2 - (operand3 + operand4) - operand5 * operand6;\n",
+               Style);
+  // clang-format on
+}
+
 } // namespace
 } // namespace test
 } // namespace format

clang/lib/Format/ContinuationIndenter.cpp

clang/unittests/Format/FormatTest.cpp

clang/lib/Format/ContinuationIndenter.cpp

github-actions · 2024-07-01T17:02:04Z

✅ With the latest revision this PR passed the C/C++ code formatter.

HazardyKnusperkeks

Could you please add a note to clang/docs/ReleaseNotes.rst.

ameerj · 2024-07-09T16:00:27Z

Thanks for the review and approval @HazardyKnusperkeks !

I don't have write permissions to the repo so please merge this for me when you have a moment.

clang/lib/Format/ContinuationIndenter.cpp

clang/lib/Format/TokenAnnotator.cpp

clang/unittests/Format/FormatTest.cpp

owenca

Also, we need ConfigParseTest for the new option.

clang/lib/Format/TokenAnnotator.cpp

clang/unittests/Format/FormatTest.cpp

owenca · 2024-07-16T02:22:19Z

This patch doesn't seem to work with the last two examples in #79487:

$ cat foo.cc
template<typename T>
concept my_concept =
       std::is_object_v<T>
    && std::is_const_v<T>
    && std::default_initializable<T>;

std::uint32_t a =
      byte_buffer[0]
    | byte_buffer[1] << 8
    | byte_buffer[2] << 16
    | byte_buffer[3] << 24;
$ clang-format -style='{BinPackBinaryOperations: false, BreakBeforeBinaryOperators: NonAssignment, ColumnLimit: 50}' foo.cc
template <typename T>
concept my_concept =
    std::is_object_v<T>
    && std::is_const_v<T> && std::default_initializable<T>;

std::uint32_t a =
    byte_buffer[0]
    | byte_buffer[1] << 8 | byte_buffer[2] << 16 | byte_buffer[3] << 24;
$

The logical && and bitwise | operations are not broken up into one per line, and the column limit is exceeded.

owenca · 2024-07-16T04:00:42Z

It seems that this patch implements the new option without regard to the relative precedence of the binary operators:

$ cat bar.cc
foooooooooooo1 = foooooooooooo2 * foooooooooooo3 +
                 foooooooooooo4 / foooooooooooo5 - foooooooooooo6;
$ clang-format -style='{BinPackBinaryOperations: false}' bar.cc
foooooooooooo1 = foooooooooooo2 *
                 foooooooooooo3 +
                 foooooooooooo4 /
                 foooooooooooo5 -
                 foooooooooooo6;
$

@mydeveloperday @HazardyKnusperkeks @rymiel are you okay with this?

HazardyKnusperkeks · 2024-07-16T19:07:22Z

It seems that this patch implements the new option without regard to the relative precedence of the binary operators:
$ cat bar.cc
foooooooooooo1 = foooooooooooo2 * foooooooooooo3 +
                 foooooooooooo4 / foooooooooooo5 - foooooooooooo6;
$ clang-format -style='{BinPackBinaryOperations: false}' bar.cc
foooooooooooo1 = foooooooooooo2 *
                 foooooooooooo3 +
                 foooooooooooo4 /
                 foooooooooooo5 -
                 foooooooooooo6;
$ 
@mydeveloperday @HazardyKnusperkeks @rymiel are you okay with this?

Per documentation that is exactly intended. So I'm in favor of that behavior.

mydeveloperday · 2024-07-17T08:42:14Z

I'm not finding the true/false every clear, can we use an enum?

owenca · 2024-07-17T15:54:48Z

It seems AlignOperands is supposed to do what this new option would, so maybe we should fix/extend AlignOperands instead?

ameerj · 2024-07-17T16:07:27Z

The logical && and bitwise | operations are not broken up into one per line, and the column limit is exceeded.

@owenca this should be fixed in the latest commit

ameerj · 2024-07-17T16:36:10Z

It seems AlignOperands is supposed to do what this new option would, so maybe we should fix/extend AlignOperands instead?

This new setting is only responsible for breaking long expressions, not aligning.
AlignOperands can be used in conjunction with this new setting for different alignment styles.

ameerj · 2024-07-17T17:28:24Z

I'm not finding the true/false every clear, can we use an enum?

Reworked this to now be an enum. Please let me know if the intent is more clear now or if anything needs further clarification.

mydeveloperday

I think the enum is clearer and leave you open to add new options as you did, thank you,

mydeveloperday

I have no major objection I'll go with whatever @owenca and @HazardyKnusperkeks agree

HazardyKnusperkeks · 2024-07-19T19:32:00Z

Fine by me. The build states:

Failed Tests (1):
Clang-Unit :: Format/./FormatTests/ConfigParseTest/ParsesConfiguration

Is this from this change?

ameerj · 2024-07-22T16:01:10Z

Failed Tests (1):

Fixed in the latest commit

ameerj · 2024-07-29T12:36:12Z

@owenca Can you review once more please?

owenca

This patch also fixes #58014 and #57280, so you can include them in the commit message.

clang/lib/Format/ContinuationIndenter.cpp

clang/include/clang/Format/Format.h

clang/unittests/Format/FormatTest.cpp

ameerj · 2024-08-09T15:28:08Z

Thanks for the review feedback @owenca @HazardyKnusperkeks
Please merge this PR for me if you feel it is ready as I don't have merge permissions

owenca · 2024-08-09T15:42:15Z

Thanks for the review feedback @owenca @HazardyKnusperkeks Please merge this PR for me if you feel it is ready as I don't have merge permissions

I wonder if anyone can come up with a better name than OnePerLine. See #101882 (comment). @mydeveloperday @HazardyKnusperkeks @rymiel? If not, I can merge this patch as is.

HazardyKnusperkeks · 2024-08-10T07:00:25Z

Nah, I can't find a better name, which isn't a whole sentence.

ameerj added 3 commits June 10, 2024 13:00

Add BinPackBinaryOperations

a50f3d4

Add BinPackBinaryOperations unit tests

1fe691c

Update ClangFormatStyleOptions.rst

d95bf09

llvmbot added clang Clang issues not falling into any other category clang-format labels Jun 10, 2024

HazardyKnusperkeks reviewed Jun 10, 2024

View reviewed changes

mydeveloperday reviewed Jun 11, 2024

View reviewed changes

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/lib/Format/ContinuationIndenter.cpp Show resolved Hide resolved

ameerj added 5 commits June 11, 2024 12:36

cleanup startsNextOperand

e9308d7

Cleanup BinPackBinaryOperations test

e58c8eb

Add another test

1de25f8

Fix mixed precedence formatting

9c9e084

Add pointer to member precedence sanity check

28ef1c7

fix formatting

6633757

HazardyKnusperkeks reviewed Jul 1, 2024

View reviewed changes

ameerj and others added 3 commits July 1, 2024 14:41

Merge remote-tracking branch 'upstream/main' into binpack-binary-ops

8d4ea2d

Update ReleaseNotes.rst

8eee170

Merge branch 'main' into binpack-binary-ops

d4e94f0

HazardyKnusperkeks approved these changes Jul 3, 2024

View reviewed changes

owenca reviewed Jul 10, 2024

View reviewed changes

clang/lib/Format/ContinuationIndenter.cpp Outdated Show resolved Hide resolved

owenca reviewed Jul 10, 2024

View reviewed changes

Review comments

daa3cf1

owenca reviewed Jul 11, 2024

View reviewed changes

clang/lib/Format/TokenAnnotator.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

ameerj added 2 commits July 11, 2024 11:25

Review comments

5520dc1

missed a trailing newline

21bcfa5

owenca reviewed Jul 15, 2024

View reviewed changes

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

clang/unittests/Format/FormatTest.cpp Outdated Show resolved Hide resolved

missed trailing newlines

e1f1cf2

owenca mentioned this pull request Jul 17, 2024

Question about alignment of binary operators #91066

Open

Fixes for BreakBeforeBinaryOperators

33a6ef9

Rename BinPackBinaryOperations -> BreakBinaryOperations enum

5b091ff

ameerj changed the title ~~[clang-format] Add BinPackBinaryOperations configuration~~ [clang-format] Add BreakBinaryOperations configuration Jul 17, 2024

mydeveloperday reviewed Jul 19, 2024

View reviewed changes

Fix typo in config parsing

0665208

ameerj added 2 commits July 23, 2024 11:02

Merge remote-tracking branch 'upstream/main' into binpack-binary-ops

3aca776

clang-format version 20

e062cff

owenca reviewed Aug 5, 2024

View reviewed changes

Review feedback

66b866e

owenca removed the clang Clang issues not falling into any other category label Aug 5, 2024

owenca approved these changes Aug 5, 2024

View reviewed changes

owenca mentioned this pull request Aug 6, 2024

[clang-format] Change BinPackParameters to enum and add AlwaysOnePerLine #101882

Merged

owenca merged commit c5a4291 into llvm:main Aug 10, 2024
9 checks passed

[clang-format] Add BreakBinaryOperations configuration #95013

[clang-format] Add BreakBinaryOperations configuration #95013

Uh oh!

Conversation

ameerj commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HazardyKnusperkeks left a comment

Choose a reason for hiding this comment

Uh oh!

ameerj commented Jul 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenca commented Jul 16, 2024

Uh oh!

owenca commented Jul 16, 2024

Uh oh!

HazardyKnusperkeks commented Jul 16, 2024

Uh oh!

mydeveloperday commented Jul 17, 2024

Uh oh!

owenca commented Jul 17, 2024

Uh oh!

ameerj commented Jul 17, 2024

Uh oh!

ameerj commented Jul 17, 2024

Uh oh!

ameerj commented Jul 17, 2024

Uh oh!

mydeveloperday left a comment

Choose a reason for hiding this comment

Uh oh!

mydeveloperday left a comment

Choose a reason for hiding this comment

Uh oh!

HazardyKnusperkeks commented Jul 19, 2024

Uh oh!

ameerj commented Jul 22, 2024

Uh oh!

ameerj commented Jul 29, 2024

Uh oh!

owenca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ameerj commented Aug 9, 2024

Uh oh!

owenca commented Aug 9, 2024

ameerj commented Jun 10, 2024 •

edited

Loading

llvmbot commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jul 1, 2024 •

edited

Loading