1338105 - [clang-format] Incorrect placement of binary operators in C++

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Jean-Yves Avenard [:jya]]

From https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

"Overlong expressions not joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line. This applies to ?:, binary arithmetic operators including +, and member-of operators (in particular the . operator in JavaScript, see the Rationale)."

So.
MOZ_ASSERT(!mChild && !(mBits & nsCachedStyleData::GetBitForSID(aSID)) &&
           !GetCachedStyleData(aSID),
           "bar");

should be:
MOZ_ASSERT(!mChild && !(mBits & nsCachedStyleData::GetBitForSID(aSID))
           && !GetCachedStyleData(aSID),
           "bar");

at the very least to be mozilla style compliant.
Currently clang-format always place the operators at the end of the line

However, provided the line is to be cut, it becomes much clearer if it is written:
MOZ_ASSERT(!mChild
           && !(mBits & nsCachedStyleData::GetBitForSID(aSID))
           && !GetCachedStyleData(aSID),
           "bar");

that is, one operation per line.
the benefit are more obvious with longer lines such as:

  return (nextKeyframe < aTimeThreshold
          || (mVideo.mTimeThreshold
              && mVideo.mTimeThreshold.ref().EndTime() < aTimeThreshold))
         && nextKeyframe.ToMicroseconds() >= 0
         && !nextKeyframe.IsInfinite();

instead of:
  return (nextKeyframe < aTimeThreshold
          || (mVideo.mTimeThreshold
              && mVideo.mTimeThreshold.ref().EndTime() < aTimeThreshold))
         && nextKeyframe.ToMicroseconds() >= 0 && !nextKeyframe.IsInfinite();

even though the above would be compliant.

Comment 1

[Andi [:andi]]

I can take this since i've already uploaded a patch to clang-format.

Comment 2

[Andi [:andi]]

I think for this one our current coding stye template from clang-format should be changed by adding:

>>MozillaStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_All;

>>  /// \brief The style of breaking before or after binary operators.
>>  enum BinaryOperatorStyle {
>>    /// Break after operators.
>>    BOS_None,
>>    /// Break before operators that aren't assignments.
>>    BOS_NonAssignment,
>>    /// Break before operators.
>>    BOS_All,
>>  };

Comment 3

[Jean-Yves Avenard [:jya]]

(In reply to Andi-Bogdan Postelnicu from comment #2)
> I think for this one our current coding stye template from clang-format
> should be changed by adding:
> 
> >>MozillaStyle.BreakBeforeBinaryOperators = FormatStyle::BOS_All;
> 
> >>  /// \brief The style of breaking before or after binary operators.
> >>  enum BinaryOperatorStyle {
> >>    /// Break after operators.
> 
BTW, this is also something wrong with clang-format
braces should be of classes, struct, construc, must be at the beginning of the next line
https://developer.mozilla.org/fr/docs/Developer_Guide/Coding_Style#Methods

so:
enum BinaryOperatorStyle
{
  BOS_None,
  BOS_NonAssignment
};

should I open another bug for this one?

Comment 4

[Andi [:andi]]

Bug with patch for clang-format: https://reviews.llvm.org/D29987

Comment 5

[Jean-Yves Avenard [:jya]]

I can't comment on the llvm website so I'm doing it here:

"This revision is addressed to fix the following situations when the correct format should be changed from:

MOZ_ASSERT(!mChild && !(mBits & nsCachedStyleData::GetBitForSID(aSID)) &&
           !GetCachedStyleData(aSID),
           "bar");

MOZ_ASSERT(!mChild && !(mBits & nsCachedStyleData::GetBitForSID(aSID))
             && !GetCachedStyleData(aSID),
           "bar");
"

this is incorrect.
To be compliant with our rules:
MOZ_ASSERT(!mChild && !(mBits & nsCachedStyleData::GetBitForSID(aSID))
           && !GetCachedStyleData(aSID),
           "bar");

The operator must be aligned with the previous argument.

However, ideally that should be:
MOZ_ASSERT(!mChild
           && !(mBits & nsCachedStyleData::GetBitForSID(aSID))
           && !GetCachedStyleData(aSID),
           "bar");

as we've started wrapping the expression, so one operand per line

Comment 6

[Andi [:andi]]

what you are saying is another bug, where the expression is aligned to the expression block from it is originated. 

This bug is only addressed to have the line break before the binary operator.

Comment 7

[Jean-Yves Avenard [:jya]]

So it appears that operator || and && should be placed at the end of lines.
But all other operators are to be placed at the beginning of the line, that include ? :

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

"Break long conditions **after** && and || logical connectives. See below for the rule for other operators."
(emphasis mine)

"Overlong expressions **not** joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line. "

so:
return (nextKeyframe < aTimeThreshold
          || (mVideo.mTimeThreshold
              && mVideo.mTimeThreshold.ref().EndTime() < aTimeThreshold))
         && nextKeyframe.ToMicroseconds() >= 0
         && !nextKeyframe.IsInfinite();

should be written:
  return (nextKeyframe < aTimeThreshold ||
          (mVideo.mTimeThreshold &&
           mVideo.mTimeThreshold.ref().EndTime()
           < aTimeThreshold)) &&
         nextKeyframe.ToMicroseconds() >= 0 &&
         !nextKeyframe.IsInfinite();

note the position of the < operator.

Personally, I don't like it, and is certainly not as readable as having *all* operators at the front, but majority rules I guess..

Comment 8

[damarnadh]

(in reply to Jean-Yves Avenard fro comment #7)

should be written:

  return (nextKeyframe < aTimeThreshold ||
          (mVideo.mTimeThreshold &&
           mVideo.mTimeThreshold.ref().EndTime() < aTimeThreshold)) &&
           nextKeyframe.ToMicroseconds() >= 0 &&
           !nextKeyframe.IsInfinite();


So it appears that operator || and && should be placed at the end of lines. so Overlong expressions not joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line.

Comment 9

[Andi [:andi]]

Agree with that but i don't think clang-format owners would allow such a particular modification only for our coding style. They have strict rules when adding flags or implementations that address only one coding style.

Comment 10

[damarnadh]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> I can take this since i've already uploaded a patch to clang-format.

can i see the patch that you are uploaded
And also i need resources to understand about clang in more detailed

Comment 11

[damarnadh]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> I can take this since i've already uploaded a patch to clang-format.

can i see the patch that you are uploaded
And also i need resources with clear examples to understand about clang in more detailed

Comment 12

[Andi [:andi]]

(In reply to damarnadh from comment #11)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> > I can take this since i've already uploaded a patch to clang-format.
> 
> can i see the patch that you are uploaded
> And also i need resources with clear examples to understand about clang in
> more detailed

Hello, you could check this patch out: https://github.com/llvm-mirror/clang/commit/c658c8caa228ebed6629ee2364f5c88c2234cbf8

Comment 13

[Emilio Cobos Álvarez (:emilio)]

I don't read that sentence in the style guide as if that should apply to || and && operators, fwiw. And actually, the coding style says the opposite:

> Break long conditions after && and || logical connectives. See below for the rule for other operators.

Also, note that there was recent discussion in the mailing list about this:

https://groups.google.com/forum/#!msg/mozilla.dev.platform/LD_KCtmmH74/bbNIz6hnBwAJ;context-place=searchin/mozilla.dev.platform/operator$20line%7Csort:relevance

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(sorry, should've read the whole bug before commenting. I think the mailing list link is worth though)

Comment 15

[Eric Rahm [:erahm]]

There's been a lot of churn in this bug and it's not clear as to what's needed at this point.

jya, can we close this bug? I see bug 1338107 for indenting the trinary operator ('?' ':') and bug 1396515 for the boolean operators. If there's something left to do here can you summarize it succinctly and possibly update the title?

Comment 16

[Jean-Yves Avenard [:jya]]

bug 1396515 is me manually updated the dom/media code, it's not about clang-format

There are several issues as I can see:

1- all binary operators are put at the end of the line, not just || and &&
2- trinary operator is wrong (bug 1338107)
3- 2nd Argument is indented by 2 spaces:
e.g.
  if (a +
        b)


So to me this bug is about fixing 1, while the original description is wrong, and || and && are correctly placed, it's not right.

I'll open a bug for item 3 shortly

Comment 17

[Andi [:andi]]

(In reply to Jean-Yves Avenard [:jya] from comment #16)
> bug 1396515 is me manually updated the dom/media code, it's not about
> clang-format
> 
> There are several issues as I can see:
> 
> 1- all binary operators are put at the end of the line, not just || and &&
Unfortunately clang-format doesn't offer a configuration specific for this case we only have:
>>  /// \brief The style of breaking before or after binary operators.
>>  enum BinaryOperatorStyle {
>>    /// Break after operators.
>>    BOS_None,
>>    /// Break before operators that aren't assignments.
>>    BOS_NonAssignment,
>>    /// Break before operators.
>>    BOS_All,
>>  }
What we could do is something specific for our case where we put the line breaker for the rest of binary operators or at least for some of them that we know for sure that we want to break on the next line before the beginning of the next token.

> 2- trinary operator is wrong (bug 1338107)
> 3- 2nd Argument is indented by 2 spaces:
> e.g.
>   if (a +
>         b)
> 
> 
> So to me this bug is about fixing 1, while the original description is
> wrong, and || and && are correctly placed, it's not right.
> 
> I'll open a bug for item 3 shortly

Comment 18

[Eric Rahm [:erahm]]

It seems like the pragmatic approach is to change this rule to follow whatever clang-format supports considering we're the only organization that wants this distinction between logical operators (`||` and `&&`) and binary operators (`<`, `!=`, etc).

I'll send an email to dev-platform to see if there is support for this.

Comment 19

[Eric Rahm [:erahm]]

Formal request to change our style guide: https://groups.google.com/d/msg/mozilla.dev.platform/a3v52PtSnWg/2x_nWekSAQAJ

Comment 20

[Andi [:andi]]

Are we still confronting with this issue since we've migrated to the Google Coding Style, if not we should close it.

Comment 21

[Jean-Yves Avenard [:jya]]

honnest answer: I don't know.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Ok, let's close it then.

We can always reopen it if we need.