Proposal: always use braces for if/for/while statements

[Jan de Mooij]

Hi all,

I'd like to propose we change the SpiderMonkey coding style to always brace
if/for/while statements.

It matches Gecko's coding style and in the past there has been agreement to
unify our coding styles as much as possible. Adding unnecessary braces
often results in style nits when Gecko hackers write SpiderMonkey patches
(or similarly, missing braces in Gecko code). The situation is worse in
XPConnect where both styles are used. Also, when to use {} can be pretty
confusing for people new to SpiderMonkey.

The downside is that it's more verbose, especially for a file like
BytecodeEmitter.cpp that has tons of unbraced if-statements (it will add
about 900-1000 lines to that file).

The conversion is pretty easy to automate - while waiting for feedback and
Try results today I wrote a simple script to do this, here's the diff for
BytecodeEmitter.cpp: https://dpaste.de/HKvM -- I still have to review the
changes more closely (and probably compare the generated object files) but
overall it looks reasonable.

Thoughts?

Thanks,
Jan

[David Teller]

I am all for making braces a requirement, both to unify with Gecko and
for safety reasons – it would be really annoying to introduce bugs, as
Apple did not that long ago, just because of missing braces.

Cheers,
David

[Steve Fink]

Heh. I find the bug argument unconvincing. It's true that the
always-brace style is slightly less error-prone, but since this is only
used for very short clauses it seems like a very minor win in practice,
and much less significant to my eye than the benefits in evening out the
code density. I find the braceless style much more readable overall --
braces add a whole bunch of visual white space that makes it harder to
parse out the logical groupings of code within a function body.

And yet, I vote "yes", because I find the consistency argument even more
convincing.

(I'd prefer Gecko to lose the braces, but that's not going to happen.)

I started out always bracing, by the way, but was won over to braceless
style through exposure to spidermonkey code. At least for 'if'
statements, anyway -- my personal leaning would be to always brace
loops. :-/

> _______________________________________________
> dev-tech-js-engine-internals mailing list
> dev-tech-js-en...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals

[Kannan Vijayan]

I'm a "yes" on this as well. I really don't care much either way in terms
of personal preference, but the consistency argument works for me as it
does for sfink.

Kannan

[Bobby Holley]

+1

The one downside of doing this now is that we'll eventually do another bulk
reformat of all of mozilla-central once we settle on a clang-format
version+config whose output we're happy with. So if that were to happen
very soon (unlikely), doing piecemeal handling of braces now would result
in extra effort and blame churn that could have been avoided. But since the
clang-format work will probably take a while, switching now will allow new
code to more-closely match long-term style, and allow everyone to get
comfortable with it.

bholley

On Tue, Sep 4, 2018 at 7:41 AM Jan de Mooij <jdem...@mozilla.com> wrote:

> Hi all,
>
> I'd like to propose we change the SpiderMonkey coding style to always brace
> if/for/while statements.
>

> It matches Gecko's coding style and in the past there has been agreement to
> unify our coding styles as much as possible. Adding unnecessary braces
> often results in style nits when Gecko hackers write SpiderMonkey patches
> (or similarly, missing braces in Gecko code). The situation is worse in
> XPConnect where both styles are used. Also, when to use {} can be pretty
> confusing for people new to SpiderMonkey.
>
> The downside is that it's more verbose, especially for a file like
> BytecodeEmitter.cpp that has tons of unbraced if-statements (it will add
> about 900-1000 lines to that file).
>
> The conversion is pretty easy to automate - while waiting for feedback and
> Try results today I wrote a simple script to do this, here's the diff for
> BytecodeEmitter.cpp: https://dpaste.de/HKvM -- I still have to review the
> changes more closely (and probably compare the generated object files) but
> overall it looks reasonable.
>
> Thoughts?
>
> Thanks,
> Jan

[Luke Wagner]

+1 for consistency

I was wondering about the timing of any big clang-format changes too; makes
sense to do this sooner than later if the clang-format changes aren't
imminent.

[Jeff Walden]

On 09/04/2018 07:50 AM, David Teller wrote:

> I am all for making braces a requirement, both to unify with Gecko and
> for safety reasons – it would be really annoying to introduce bugs, as
> Apple did not that long ago, just because of missing braces.

I say this all with a bit of a smile (in case it is not obvious), but:

Bracing single-line statements is a trash rule.

"Safety" is a trash justification. The proper solution to the "safety" concern is to enforce an indentation rule that absolutely addresses the trash situation Apple had.

The hyper-fixation of brace-everything defenders on a single case gone wrong -- without ever considering that enforcing *other*, utterly un-controversial, existing style rules would equally have addressed that concern and every other "safety" concern bracing moots -- is a myopia of "safety" over readability and noise reduction.

And IMO readability on every single line of source code, even outweighs the false "safety" or bug-avoidance supposition of always-bracing.

Gecko absolutely should change their trash rule and not require single-line bracing by enforcing indentation requirements.

But the utterly uncontroversial fact that Gecko's style is trash is not great enough reason to not join them. Mozilla has long had a problem of our never being able to converge on single solutions for things (IRC, Slack; Mercurial, git, svn, I think even bzr or arch in one or two places; Bugzilla, Service Now, Github, other request tracking; and so on). It is better to encourage standardization at cost in denying Inescapable Fact, than to actually be right.

Jeff

[David Teller]

Point taken. Not going to fight over this :)

[Jeff Walden]

On 09/04/2018 10:59 AM, David Teller wrote:
> Point taken. Not going to fight over this :)

:-) ^ 2

Jeff

[Benjamin Bouvier]

I don't buy Jeff's argument at all; in the case of the (in)famous Apple SSL
bug, there *was* proper indentation and missing braces on the if, and
though this wasn't enough to catch the failure. Meta bug 1297639 has
dependents that show real-world issues because of missing braces, including
in Gecko.

I'm all in for the new rule:
- for consistency with Gecko (and future happy tooling with clang-format)
- less cognitive load for contributors
- harder to make a mistake
- tools to catch these errors automatically are imperfect at the moment: if
the if-body-one-liner is a macro that expands on several lines (without
braces), gcc 7.3 with -Wmisleading-indentation doesn't catch it [1]. (of
course it could be fixed in gcc itself, but that means being unprotected
until we get this feature in stable GCC, and GCC updated in all the major
distributions; note there might issues other than macros which I haven't
thought of)

Cheers,
Benjamin

[1] example: https://godbolt.org/z/-iepQl

[Ehsan Akhgari]

On Tue, Sep 4, 2018 at 12:49 PM Bobby Holley <bobby...@gmail.com> wrote:

> +1
>
> The one downside of doing this now is that we'll eventually do another bulk
> reformat of all of mozilla-central once we settle on a clang-format
> version+config whose output we're happy with. So if that were to happen
> very soon (unlikely), doing piecemeal handling of braces now would result
> in extra effort and blame churn that could have been avoided. But since the
> clang-format work will probably take a while, switching now will allow new
> code to more-closely match long-term style, and allow everyone to get
> comfortable with it.
>

It's not quite clear when the clang-format conversion will happen yet and
what it will look like yet, but I can tell you that it isn't *imminent* (as
in, not in the next couple of weeks for example), in case that helps making
the decision here.

Cheers,
Ehsan

> > _______________________________________________
> > dev-tech-js-engine-internals mailing list
> > dev-tech-js-en...@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
> >
> _______________________________________________
> dev-tech-js-engine-internals mailing list
> dev-tech-js-en...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
>

--
Ehsan

[Jeff Walden]

On 09/05/2018 02:29 AM, Benjamin Bouvier wrote:
> I don't buy Jeff's argument at all; in the case of the (in)famous Apple SSL
> bug, there *was* proper indentation and missing braces on the if, and
> though this wasn't enough to catch the failure.

*Having* the rule is not the same as *enforcing* it by linter (or that compiler option), and I meant enforcing the rule, not merely stating it and seeing what happens.

GCC/clang limitations don't amount to much, either, IMO. It isn't *that* hard to write a fairly dumb checker-script to enforce indentation rules. GCC and clang are not a serious limit here, and we could fix them if it came to it.

Jeff

[Jan de Mooij]

Patches converting most of js/src have landed and the style guide has been
updated. I'll post patches for the few remaining directories and XPConnect
this week.

Don't worry too much about fixing up in-flight patches; I'll run the script
again in a few days.

Thanks,
Jan

[Kent Williams]

I understand the urge to have consistent formatting, and that ideas
about the best way to format evolve over time.

Here's the A#1 reason this is NOT necessarily a good idea: It obscures
the real changes when you compare versions.

If you want to change formatting standards, knock yourselves out. But I
would propose you think about adding another tool to the arsenal:

Since you are going to automate the formatting change also implement a
filter -- I believe you can install it as a git or mercurial or whatever
extension -- that will reformat the old version of the code before
comparing to the new version, so that the formatting changes are taken
out of the diff output.

I generally do diffs with --ignore-all-space; this would be a stronger
option, that reformats the old code to match the new code's format
before diffing.

[Bobby Holley]

On Fri, Oct 19, 2018 at 6:40 AM Kent Williams <kwil...@leepfrog.com>
wrote:

> I understand the urge to have consistent formatting, and that ideas
> about the best way to format evolve over time.
>
> Here's the A#1 reason this is NOT necessarily a good idea: It obscures
> the real changes when you compare versions.
>
> If you want to change formatting standards, knock yourselves out. But I
> would propose you think about adding another tool to the arsenal:
>
> Since you are going to automate the formatting change also implement a
> filter -- I believe you can install it as a git or mercurial or whatever
> extension -- that will reformat the old version of the code before
> comparing to the new version, so that the formatting changes are taken
> out of the diff output.
>

The problem with this approach is that the code ends up inconsistent until
every line of code has been modified for some other reason. Inconsistent
style distracts readers and confuses patch authors, which is why we haven't
done it in the past.

It's worth noting that searchfox is very good at jumping past reformats
when looking up blame.