Coding style: Placement of binary operators when breaking a long line

[Eric Rahm]

Hi folks-

*Note: Previously we've discussed the placement of logical operators && and
||; a decision was made and I do not wish to re-litigate that here*.

Currently we have a somewhat convoluted set of rules about where to place
boolean operators when breaking long lines [1]. Essentially we say that
logical operators such as `&&` stay at the end, but boolean opertators such
as `>` move to the new line. It's possible I'm misreading that (others
certainly have [2]).

This is incompatible with tools like clang-format [3] which support either
leaving the operator at the end of the line or moving it to the new line,
but not a half and half approach. It is very unlikely we could convince
them to upstream such a change given we are the only organization that has
requested it.

I would like to propose that we adjust our style guide to remove the
distinction between logical operators and boolean operators for the purpose
of line breaking and use a unified style.

*Examples*

Current rules:

if ((aArgument == 100) &&
(anotherReallyLongArgument
> 2000)) {

Proposed change if we unify the rules, favoring the logical operator
placement:

if ((aArgument == 100) &&
(anotherReallyLongArgument >
2000)) {

I want to emphasize this is a *pragmatic* change, our style guide is out of
the norm, and the ability of our developers being able to use tools like
clang-format is a big win.

Please let me know what you think.

-e

[1]
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1396515
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1338105#c16

[Mike Hommey]

On a personal note, I find > 2000 as in the first sample more readable
than the latter. So much so that I'd actually prefer the logical
operators to be on the next line than boolean operator being on the
previous.

Mike

[Eric Rahm]

As I said, I was hoping to avoid rehashing this point, but the general
consensus from the last rather contentious post [1] was that changing from
the prevalent style of the codebase for primarily aesthetic reasons was
hard to justify (points about readability were made on both sides). Nick
pointed out that our code base very clearly tilts to the operators on the
end of the line style [2].

Carrying forward this wisdom for this case makes sense, but I'll admit
moving everything to the newline is certainly an option that would provide
a unified style which is my greater concern. I don't think we could
reasonably make that change until some sort of flag day when we convert
everything as the new style would clash with the prevalent style. I also
don't want to start a discussion about that again :)

-e

[1]
https://groups.google.com/d/msg/mozilla.dev.platform/LD_KCtmmH74/dLckkuFjBwAJ
[2]
https://groups.google.com/d/msg/mozilla.dev.platform/LD_KCtmmH74/ErPunKZ_BwAJ

[Tom Ritter]

On Wed, Sep 6, 2017 at 7:18 PM, Mike Hommey <m...@glandium.org> wrote:
> On a personal note, I find > 2000 as in the first sample more readable
> than the latter. So much so that I'd actually prefer the logical
> operators to be on the next line than boolean operator being on the
> previous.

A small +1. =)

-tom

[Ehsan Akhgari]

On 09/06/2017 09:03 PM, Eric Rahm wrote:
> As I said, I was hoping to avoid rehashing this point, but the general
> consensus from the last rather contentious post [1] was that changing
> from the prevalent style of the codebase for primarily aesthetic
> reasons was hard to justify (points about readability were made on
> both sides). Nick pointed out that our code base very clearly tilts to
> the operators on the end of the line style [2].

This has already been discussed to a great length as you note Eric, and
like you I don't think rehashing the previous discussions is
particularly helpful.

Also, while I understand that people will certainly have (sometimes
strongly held) personal preferences, the Mozilla Coding Style isn't a
place where everyone's personal preferences can win, for the obvious
reason that these personal preferences are often mutually exclusive.

The interesting points to consider is the data that Nick alluded to in
the previous discussion about the existing prevalent style.

Also, the point you up about the pragmatic aspect of the need to be able
to use automated tools in order to manage our Coding Style is right on.
As things stand, the only viable option of such a tool that I'm aware of
is clang-format, and given the standing situation with regards to what
formatting options we can make that tool support in this case, I think
the pragmatic decision is to pick *one* option here for the placement of
operators across line breaks: either at the end of long lines or in the
beginning of the next line.

The combination of the above points makes me prefer adopting the
proposal to treat all operators like && and ||.

> Carrying forward this wisdom for this case makes sense, but I'll admit
> moving everything to the newline is certainly an option that would
> provide a unified style which is my greater concern. I don't think we
> could reasonably make that change until some sort of flag day when we
> convert everything as the new style would clash with the prevalent
> style. I also don't want to start a discussion about that again :)

Yes, I think once we get to a point where we format our code using
clang-format it would be much easier to argue for changes to the Coding
Style like this, but a discussion about that right now seems premature.

> -e
>
> [1]
> https://groups.google.com/d/msg/mozilla.dev.platform/LD_KCtmmH74/dLckkuFjBwAJ
> [2]
> https://groups.google.com/d/msg/mozilla.dev.platform/LD_KCtmmH74/ErPunKZ_BwAJ
>

> On a personal note, I find > 2000 as in the first sample more readable
> than the latter. So much so that I'd actually prefer the logical
> operators to be on the next line than boolean operator being on the
> previous.
>

> Mike
>
>

[Chris Peterson]

On 2017-09-06 8:06 PM, Ehsan Akhgari wrote:
> The interesting points to consider is the data that Nick alluded to in
> the previous discussion about the existing prevalent style.
>
> Also, the point you up about the pragmatic aspect of the need to be able
> to use automated tools in order to manage our Coding Style is right on.
> As things stand, the only viable option of such a tool that I'm aware of
> is clang-format, and given the standing situation with regards to what
> formatting options we can make that tool support in this case, I think
> the pragmatic decision is to pick *one* option here for the placement of
> operators across line breaks: either at the end of long lines or in the
> beginning of the next line.
>
> The combination of the above points makes me prefer adopting the
> proposal to treat all operators like && and ||.

There are only a couple hundred instances of boolean operators after a
line break in mozilla-central C++ code. I know this search is inexact,
but it shows the (small) scale of this usage compared to the proposed
before-line-break style.

https://searchfox.org/mozilla-central/search?q=%5E%5Cs%2B(%3E%7C%3E%3D%7C%3C%7C%3C%3D%7C%3D%3D%7C!%3D)%5Cs%2B(%5Cw%7C%5C()&case=true&regexp=true&path=.c

[Mike Hommey]

Note that there seems to be about the same number of boolean operators
before a line break (if you ignore the ones from generated code)

https://searchfox.org/mozilla-central/search?q=%5Cs(%3E%7C%3E%3D%7C%3C%7C%3C%3D%7C%3D%3D%7C!%3D)%5Cs*%24&case=true&regexp=true&path=.c

Mike

[Jean-Yves Avenard]

On Thu, Sep 7, 2017 at 3:03 AM, Eric Rahm <er...@mozilla.com> wrote:
> As I said, I was hoping to avoid rehashing this point, but the general
> consensus from the last rather contentious post [1] was that changing from
> the prevalent style of the codebase for primarily aesthetic reasons was
> hard to justify (points about readability were made on both sides). Nick
> pointed out that our code base very clearly tilts to the operators on the
> end of the line style [2].

Seeing that the plan is after 57, to run clang-format on the entire
codebase (with the exclusions of 3rd party code).
Do we really need to care on what the current code mainly use?

I don't think it matters, seeing that no matter what, the great
majority of the code will change, as nothing is following the current
coding style to the letter (with the exception of the majority of
dom/media)

[Ehsan Akhgari]

On 09/07/2017 04:17 AM, Mike Hommey wrote:
> On Thu, Sep 07, 2017 at 12:47:51AM -0700, Chris Peterson wrote:

> Note that there seems to be about the same number of boolean operators
> before a line break (if you ignore the ones from generated code)
>
> https://searchfox.org/mozilla-central/search?q=%5Cs(%3E%7C%3E%3D%7C%3C%7C%3C%3D%7C%3D%3D%7C!%3D)%5Cs*%24&case=true&regexp=true&path=.c

Not really. Searchfox caps the regex results at 1000. Local grep is
much more suitable for this kind of analysis. On local inbound I have:

$ git grep '\s(>|>=|<|<=|==|!=)\s*$' -- '*.c*' | wc -l
2794
$ git grep '^\s+(>|>=|<|<=|==|!=)\s+(\w|\()' -- '*.c*' | wc -l
419

This of course doesn't include all operators that matter here but the
trend is pretty clear.

[Ehsan Akhgari]

On 09/07/2017 07:23 AM, Jean-Yves Avenard wrote:
> On Thu, Sep 7, 2017 at 3:03 AM, Eric Rahm <er...@mozilla.com> wrote:
>> As I said, I was hoping to avoid rehashing this point, but the general
>> consensus from the last rather contentious post [1] was that changing from
>> the prevalent style of the codebase for primarily aesthetic reasons was
>> hard to justify (points about readability were made on both sides). Nick
>> pointed out that our code base very clearly tilts to the operators on the
>> end of the line style [2].
>
> Seeing that the plan is after 57, to run clang-format on the entire
> codebase (with the exclusions of 3rd party code).
> Do we really need to care on what the current code mainly use?

Yes, we do. The clang-format configuration we use has for the most part
been derived from the current Coding Style and the prevalent patterns
existing in our code base. Where possible we have been modifying
clang-format according to our needs. We have indeed upstreamed the
Mozilla style to clang-format for this very reason:
<http://clang.llvm.org/docs/ClangFormatStyleOptions.html>.

Post the switch to using clang-format tree-wide, our Coding Style for
matters that clang-format can automate (such as whitespace formatting
issues) will be simplified to "run clang-format". :-)

> I don't think it matters, seeing that no matter what, the great
> majority of the code will change, as nothing is following the current
> coding style to the letter (with the exception of the majority of
> dom/media)

The great majority of code changing is quite expected for any project
switching to clang-format, since as it turns out automated tools are
much better at doing this grunt work than humans are. The reason
projects choose to switch to using clang-format is increasing developer
productivity by allowing editor/IDE integration for formatting the code
as you're editing it, ensuring the code formatting remains consistent
over time without needing to spend invaluable engineering time on it,
and being able to stop debating whitespace issues and moving on to focus
on more productive discussions. ;-)

Still, picking styles that are not completely different with your
existing code matters in the sense of reducing the pain of going through
the blames when delving through the history of old code. To give you an
extreme example, if we were hypothetically to pick tabs as indentation
characters in lieu of spaces, that would basically guarantee touching
almost every line of our code no matter what, whereas the current
massive restyle of the code doesn't go anywhere close to being as
drastic of a change.

[Martin Thomson]

On Fri, Sep 8, 2017 at 1:08 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
> The great majority of code changing is quite expected for any project
> switching to clang-format, since as it turns out automated tools are much
> better at doing this grunt work than humans are. The reason projects choose
> to switch to using clang-format is increasing developer productivity by
> allowing editor/IDE integration for formatting the code as you're editing
> it, ensuring the code formatting remains consistent over time without
> needing to spend invaluable engineering time on it, and being able to stop
> debating whitespace issues and moving on to focus on more productive
> discussions. ;-)

I am 100% behind this idea. NSS is already there and it's so nice.

One question, because it hurts me every day now, which version of
clang-format will we use? NSS actually has checks in CI that will
fail if you submit unformatted code (it gives you a nice patch you can
download and apply to fix the problem even), so we're very particular
about this. However, my local version of clang-format is more recent.
There are small differences in its handling of certain constructs.
It's been screwing up my pre-commit hook for a while now (I just
haven't gotten around to fixing it just yet).

e.g.,

3.9
- CheckAcks(client_filters_, 0, {0, // SH
- 0x0002000000000000ULL, // EE
- 0x0002000000000002ULL} // CT2
4.0
+ CheckAcks(client_filters_, 0,
+ {0, // SH
+ 0x0002000000000000ULL, // EE
+ 0x0002000000000002ULL} // CT2

Have we had the version discussion yet?

[Mats Palmgren]

On 9/7/17 4:45 PM, Ehsan Akhgari wrote:
> $ git grep '\s(>|>=|<|<=|==|!=)\s*$' -- '*.c*' | wc -l
> 2794

Don't you get plenty of false positives for '>' with that regexp though?
For example:
dom/bindings/BindingUtils.h:template<typename T >
js/public/HashTable.h:struct IsPod<js::HashMapEntry<K, V> >
etc

/Mats

[Mats Palmgren]

On 9/7/17 4:45 PM, Ehsan Akhgari wrote:

> $ git grep '\s(>|>=|<|<=|==|!=)\s*$' -- '*.c*' | wc -l
> 2794

[Sylvestre Ledru]

Le 08/09/2017 à 02:24, Martin Thomson a écrit :
> On Fri, Sep 8, 2017 at 1:08 AM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
>> The great majority of code changing is quite expected for any project
>> switching to clang-format, since as it turns out automated tools are much
>> better at doing this grunt work than humans are. The reason projects choose
>> to switch to using clang-format is increasing developer productivity by
>> allowing editor/IDE integration for formatting the code as you're editing
>> it, ensuring the code formatting remains consistent over time without
>> needing to spend invaluable engineering time on it, and being able to stop
>> debating whitespace issues and moving on to focus on more productive
>> discussions. ;-)
> I am 100% behind this idea. NSS is already there and it's so nice.
>
> One question, because it hurts me every day now, which version of
> clang-format will we use?

I landed a change to use clang-format 5.0 (we were using a svn version) [1]
This can be used with:
./mach clang-format (will run on a diff)
./mach clang-format -p <path> (will run on a path/file)

Some changes landed in 5.0 to manage the Mozilla coding style out of the box (and some options
like SplitEmptyFunction only exist in this version).

Sylvestre
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1397457

[Eric Rahm]

Thanks everyone!

This got a bit derailed but it's clear that a unified style for binary
operators is desired.

I received several emails off-list in support of my original proposal, a
few people reiterated their preference for operators on the newline and we
rehashed that conversation. Overall given the back and forth it seems like
we're leaning towards going with unifying the style for splitting binary
operators by leaving them on the original line.

At this point I'll leave it to Ehsan, the code-style module owner, to make
the final decision.

-e

[Ehsan Akhgari]

On 09/11/2017 06:46 PM, Eric Rahm wrote:
> Thanks everyone!
>
> This got a bit derailed but it's clear that a unified style for binary
> operators is desired.
>
> I received several emails off-list in support of my original proposal,
> a few people reiterated their preference for operators on the newline
> and we rehashed that conversation. Overall given the back and forth it
> seems like we're leaning towards going with unifying the style for
> splitting binary operators by leaving them on the original line.
>
> At this point I'll leave it to Ehsan, the code-style module owner, to
> make the final decision.

Apologies for the late response, I was out most of the last week sick.

I think given the practicalities of enforcing the style that we want
using clang-format and the strong preference that people seem to have
for logical && and || operators being at the end of lines, we should
unify our coding style to leave all binary operators on the original
line when breaking lines. I've edited the Coding Style accordingly.

Cheers,
Ehsan

>
> -e
>
> On Wed, Sep 6, 2017 at 12:30 PM, Eric Rahm <er...@mozilla.com

> <mailto:er...@mozilla.com>> wrote:
>
> Hi folks-
>
> */Note: Previously we've discussed the placement of logical

> operators && and ||; a decision was made and I do not wish to

> re-litigate that here/*.

> <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators>
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1396515

> <https://bugzilla.mozilla.org/show_bug.cgi?id=1396515>
> [3] https://bugzilla.mozilla.org/show_bug.cgi?id=1338105#c16

> <https://bugzilla.mozilla.org/show_bug.cgi?id=1338105#c16>
>
>