Closed Bug 1338105 Opened 8 years ago Closed 6 years ago

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

Categories

(Developer Infrastructure :: Lint and Formatting, defect)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jya, Assigned: andi)

References

(Blocks 1 open bug)

Details

Assignee: nobody → bpostelnicu
I can take this since i've already uploaded a patch to clang-format.
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, >> };
(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?
Bug with patch for clang-format: https://reviews.llvm.org/D29987
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
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.
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..
Flags: needinfo?(jyavenard)
Flags: needinfo?(bpostelnicu)
Flags: needinfo?(jyavenard)
Flags: needinfo?(bpostelnicu)
(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.
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.
(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
(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
(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
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
(sorry, should've read the whole bug before commenting. I think the mailing list link is worth though)
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?
Flags: needinfo?(jyavenard)
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
Flags: needinfo?(jyavenard)
Summary: [clang-format] Incorrect placement of operator in C++ → [clang-format] Incorrect placement of binary operators in C++
(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
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.
Product: Core → Firefox Build System
Component: Source Code Analysis → Lint and Formatting

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

Flags: needinfo?(jyavenard)

honnest answer: I don't know.

Flags: needinfo?(jyavenard)

Ok, let's close it then.

We can always reopen it if we need.

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.