A proposal to reduce the number of styles in Mozilla code

[Nicholas Nethercote]

We've had some recent discussions about code style. I have a propasal

For the purpose of this proposal I will assume that there is consensus on the
following ideas.

- Having multiple code styles is bad.

- Therefore, reducing the number of code styles in our code is a win (though
there are some caveats relating to how we get to that state, which I discuss
below).

- The standard Mozilla style is good enough. (It's not perfect, and it should
continue to evolve, but if you have any pet peeves please mention them in a
different thread to this one.)

With these ideas in mind, a goal is clear: convert non-Mozilla-style code to
Mozilla-style code, within reason.

There are two notions that block this goal.

- Our rule of thumb is to follow existing style in a file. From the style
guide:

"The following norms should be followed for new code, and for Tower of Babel
code that needs cleanup. For existing code, use the prevailing style in a
file or module, or ask the owner if you are on someone else's turf and it's
not clear what style to use."

This implies that large-scale changes to convert existing code to standard
style are discouraged. (I'd be interested to hear if people think this
implication is incorrect, though in my experience it is not.)

I propose that we officially remove this implicit discouragement, and even
encourage changes that convert non-Mozilla-style code to Mozilla-style (with
some exceptions; see below). When modifying badly-styled code, following
existing style is still probably best.

However, large-scale style fixes have the following downsides.

- They complicate |hg blame|, but plenty of existing refactorings (e.g.
removing old types) have done likewise, and these are bearable if they
aren't too common. Therefore, style conversions should do entire files in
a single patch, where possible, and such patches should not make any
non-style changes. (However, to ease reviewing, it might be worth
putting fixes to separate style problems in separate patches. E.g. all
indentation fixes could be in one patch, separate from other changes.
These would be combined before landing. See bug 956199 for an example.)

- They can bitrot patches. This is hard to avoid.

However, I imagine changes would happen in a piecemeal fashion, e.g. one
module or directory at a time, or even one file at a time. (Again, see bug
956199 for an example.) A gigantic change-all-the-code patch seems
unrealistic.

- There is an semi-official policy that the owner of a module can dictate its
style. Examples: SpiderMonkey, Storage, MFBT.

There appears to be no good reason for this and I propose we remove it.
Possibly with the exception of SpiderMonkey (and XPConnect?), due to it being
an old and large module with its own well-established style.

Also, we probably shouldn't change the style of imported third-party code;
even if we aren't tracking upstream, we might still want to trade patches.
(Indeed, it might even be worth having some kind of marking at the top of
files to indicate this, a bit like a modeline?)

Finally, this is a proposal only to reduce the number of styles in our
codebase. There are other ideas floating around, such as using automated tools
to enforce consistency, but I consider them orthogonal to or
follow-ups/refinements of this proposal -- nothing can happen unless we agree
on a direction (fewer styles!) and a way to move in that direction (non-trivial
style changes are ok!)

Nick

[smaug]

Sounds good, and I'd include also js/* so that we had consistent style everywhere.
It is rather painful to hack various non-js/* and js/* (xpconnect in my case)
in the same patch.
(I also happen to think that Mozilla coding style is inherently better than js style, since
it has clear rules for naming parameters and static, global and member variables in a way
which make them look different to local variables.)


3rd party code shouldn't be touched.


-Olli

[Gavin Sharp]

A concise summary of the changes you're proposing would be useful -
here's my attempt at one.

>From what I gather, the changes you're proposing to the style guide are:

* remove implicit discouragement of changing code to conform to "Mozilla style"
** style changes should never be combined with functional changes
** (other specifics about how this should be accomplished and with
what exceptions may need elaborating)
* remove the text suggesting that module owners can dictate different
code styles

I don't have any objection, and think that these policy changes are
reasonable. I think the work that these policy changes entail (i.e.
individual style fix patches) should still be evaluated on its own
merit by module owners/peers, and that may mean that the style fixes
are treated as a lower priority than other work.

I think we should also do a review of the style guide before we decide
to try to enforce it more consistently. I've done that quickly and
have noticed a couple of issues - I will post separately about that.

Gavin

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Adam Roach]

On 1/6/14 09:50, Gavin Sharp wrote:
> A concise summary of the changes you're proposing would be useful -
> here's my attempt at one.
>
> From what I gather, the changes you're proposing to the style guide are:
>
> * remove implicit discouragement of changing code to conform to "Mozilla style"
> ** style changes should never be combined with functional changes
> ** (other specifics about how this should be accomplished and with
> what exceptions may need elaborating)
> * remove the text suggesting that module owners can dictate different
> code styles

You missed:

* Don't reformat third-party code.

/a

[Gavin Sharp]

I was listing only the policy changes, and would have lumped that one into

"(other specifics about how this should be accomplished and with

what exceptions may need elaborating)", but yes, it's certainly worth
calling out explicitly.

Gavin

[Joshua Cranmer 🐧]

On 1/5/2014 8:34 PM, Nicholas Nethercote wrote:
> With these ideas in mind, a goal is clear: convert non-Mozilla-style code to
> Mozilla-style code, within reason.

One thing that is worth pointing out is that there are three distinct
classes of style rules, which have different implications for conversion
and unification of guidelines:

* Formatting changes, e.g., number of characters in a line, indent
width, brace positioning. By definition, these are invisible to the
compiler (for any language other than whitespace-sensitive languages
like Python or Make), so there are no issues with reformatting
automatically.

* Naming conventions. This is where our multitude of styles is most
evident, but it also creates compatibility issues, particularly for code
that is part of the public API (namely, JS code and XPIDL binary code
generation).

* Other structural rules. Examples here include situations like "don't
use NULL, use nullptr", "always brace an if/else/for/while statement",
etc. These rules are harder to automatically enforce.

>
> There are two notions that block this goal.
>
> - Our rule of thumb is to follow existing style in a file. From the style
> guide:
>
> "The following norms should be followed for new code, and for Tower of Babel
> code that needs cleanup. For existing code, use the prevailing style in a
> file or module, or ask the owner if you are on someone else's turf and it's
> not clear what style to use."
>
> This implies that large-scale changes to convert existing code to standard
> style are discouraged. (I'd be interested to hear if people think this
> implication is incorrect, though in my experience it is not.)
>
> I propose that we officially remove this implicit discouragement, and even
> encourage changes that convert non-Mozilla-style code to Mozilla-style (with
> some exceptions; see below). When modifying badly-styled code, following
> existing style is still probably best.
>
> However, large-scale style fixes have the following downsides.
>
> - They complicate |hg blame|, but plenty of existing refactorings (e.g.
> removing old types) have done likewise, and these are bearable if they
> aren't too common. Therefore, style conversions should do entire files in
> a single patch, where possible, and such patches should not make any
> non-style changes. (However, to ease reviewing, it might be worth
> putting fixes to separate style problems in separate patches. E.g. all
> indentation fixes could be in one patch, separate from other changes.
> These would be combined before landing. See bug 956199 for an example.)

Whitespace-only changes are easily ignorable in blame tooling, and I
thus consider them a non-issue, particularly since the CVS/hg blame
divide still hurts me when doing historical blame research.

> - There is an semi-official policy that the owner of a module can dictate its
> style. Examples: SpiderMonkey, Storage, MFBT.
>
> There appears to be no good reason for this and I propose we remove it.
> Possibly with the exception of SpiderMonkey (and XPConnect?), due to it being
> an old and large module with its own well-established style.

A related issue that is worth bringing up is that we have wildly
different style guides for different languages. The effective style for
Mozilla code I've worked with has been "brace on newline for C/C++ code
but at end of line for JS code" (style has changed somewhat I think in
general, but the "use prevailing style" rule has really confounded the
issue).

> Finally, this is a proposal only to reduce the number of styles in our
> codebase. There are other ideas floating around, such as using automated tools
> to enforce consistency, but I consider them orthogonal to or
> follow-ups/refinements of this proposal -- nothing can happen unless we agree
> on a direction (fewer styles!) and a way to move in that direction (non-trivial
> style changes are ok!)

Style guidelines are only effective if they can be enforced. Without
education of reviewers and probably automated enforcement tools, any new
style regime will only last a short while before style proliferation
happens again.

--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist

[Martin Thomson]

I think that this is a good start, but it doesn’t go quite far enough.

Part of the problem with a policy that requires people to avoid reformatting of stuff they don’t touch is that it propagates formatting divergence. Sometimes because it’s better to conform to the style immediately adjacent the changed, but more so because it prevents the use of tools that reformat entire files (here, I’m not talking about things like names, but more whitespace conventions).

For whitespace at least, I think that we need to do the following:

1. pick a style

I really, really don’t care what this is. I’m thinking that we pick whatever people think that the current style is and give folks a fixed period to debate changes.

2. create some tools

These tools should help people conform to the style.

Primarily, what is needed is a tool with appropriate configuration that runs on the command line — e.g., mach reformat … clang-format is looking like a good candidate for C/C++, it just needs a configuration. For JavaScript, I’ve used the python js-beautify, but it’s pretty light on configuration options, it might need some hacking to make it better.

Ideally though, there should be set of configuration files for common editors. I’m certain there are plenty out there already. Let’s bring them all together (attaching the files https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style might be enough).

3. reformat everything

Take the command line tool and run it over all the code. I realise that this is the contentious part, but you don’t get the real benefits until you do this.

Once you do this, it’s safe to reformat files at any time without messing with parts of files that you haven’t touched. This is important because many tools only reformat entire files.

As for `hg blame` and related tools, I have a workaround for that. I’ve knocked together a tool that takes a file and a reformatter command line, and churns out a series of patches that retain blame: https://github.com/martinthomson/blame-bridge

The patch bitrot problem is not easy to work around, but that will depend on how closely the affected files already conformed to the style guide.

4. enforce compliance

This is probably a step for the future, but if there was - for example - a commit bot that waited for a clean build+test run, adding a format check to that run would allow the bot to block patches that screwed up the formatting of files.

—Martin

On 2014-01-05, at 18:34, Nicholas Nethercote <n.neth...@gmail.com> wrote:

> We've had some recent discussions about code style. I have a propasal
>
> For the purpose of this proposal I will assume that there is consensus on the
> following ideas.
>
> - Having multiple code styles is bad.
>
> - Therefore, reducing the number of code styles in our code is a win (though
> there are some caveats relating to how we get to that state, which I discuss
> below).
>
> - The standard Mozilla style is good enough. (It's not perfect, and it should
> continue to evolve, but if you have any pet peeves please mention them in a
> different thread to this one.)
>

> With these ideas in mind, a goal is clear: convert non-Mozilla-style code to
> Mozilla-style code, within reason.
>

> There are two notions that block this goal.
>
> - Our rule of thumb is to follow existing style in a file. From the style
> guide:
>
> "The following norms should be followed for new code, and for Tower of Babel
> code that needs cleanup. For existing code, use the prevailing style in a
> file or module, or ask the owner if you are on someone else's turf and it's
> not clear what style to use."
>
> This implies that large-scale changes to convert existing code to standard
> style are discouraged. (I'd be interested to hear if people think this
> implication is incorrect, though in my experience it is not.)
>
> I propose that we officially remove this implicit discouragement, and even
> encourage changes that convert non-Mozilla-style code to Mozilla-style (with
> some exceptions; see below). When modifying badly-styled code, following
> existing style is still probably best.
>
> However, large-scale style fixes have the following downsides.
>
> - They complicate |hg blame|, but plenty of existing refactorings (e.g.
> removing old types) have done likewise, and these are bearable if they
> aren't too common. Therefore, style conversions should do entire files in
> a single patch, where possible, and such patches should not make any
> non-style changes. (However, to ease reviewing, it might be worth
> putting fixes to separate style problems in separate patches. E.g. all
> indentation fixes could be in one patch, separate from other changes.
> These would be combined before landing. See bug 956199 for an example.)
>

> - They can bitrot patches. This is hard to avoid.
>
> However, I imagine changes would happen in a piecemeal fashion, e.g. one
> module or directory at a time, or even one file at a time. (Again, see bug
> 956199 for an example.) A gigantic change-all-the-code patch seems
> unrealistic.
>

> - There is an semi-official policy that the owner of a module can dictate its
> style. Examples: SpiderMonkey, Storage, MFBT.
>
> There appears to be no good reason for this and I propose we remove it.
> Possibly with the exception of SpiderMonkey (and XPConnect?), due to it being
> an old and large module with its own well-established style.
>

> Also, we probably shouldn't change the style of imported third-party code;
> even if we aren't tracking upstream, we might still want to trade patches.
> (Indeed, it might even be worth having some kind of marking at the top of
> files to indicate this, a bit like a modeline?)
>

> Finally, this is a proposal only to reduce the number of styles in our
> codebase. There are other ideas floating around, such as using automated tools
> to enforce consistency, but I consider them orthogonal to or
> follow-ups/refinements of this proposal -- nothing can happen unless we agree
> on a direction (fewer styles!) and a way to move in that direction (non-trivial
> style changes are ok!)
>

[Bobby Holley]

I think trying to change SpiderMonkey style to Gecko isn't a great use of
effort:
* There are a lot of SpiderMonkey hackers who don't hack on anything else,
and don't consider themselves Gecko hackers. Many of them don't read
dev-platform, and would probably revolt if this were to occur.
* SpiderMonkey is kind of an external module, with an existing community of
embedders who use its style.
* SpiderMonkey has an incredibly detailed style guide - more detailed than
Gecko's: https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style
* The stylistic consistency and attention to detail is probably the highest
out of any other similarly-sized module in the tree.

This leaves us with the question about what to do with XPConnect. It used
to have its own (awful) style, and I converted it to SpiderMonkey style 2
years ago (at the then-module-owner's request). It interacts heavily with
the JS engine, so this sorta makes sense, but there's also an argument for
converting it to Gecko style. I could perhaps be persuaded at some point if
someone wants to do the leg work.

bholley

[Axel Hecht]

Hi,

two points of caution:

In the little version control archaeology I do, I hit "breaks blame for
no good reason" pretty often already. I'd not underestimate the cost for
the project of doing changes just for the sake of changes.

Tools don't get code right. I've seen various changes where tools
reformat code they don't understand, and break it. Trailing whitespace
is significant in some of our file formats, for example.

Axel

On 1/6/14 6:55 PM, Martin Thomson wrote:
> I think that this is a good start, but it doesn�t go quite far enough.
>
> Part of the problem with a policy that requires people to avoid reformatting of stuff they don�t touch is that it propagates formatting divergence. Sometimes because it�s better to conform to the style immediately adjacent the changed, but more so because it prevents the use of tools that reformat entire files (here, I�m not talking about things like names, but more whitespace conventions).

>
> For whitespace at least, I think that we need to do the following:
>
> 1. pick a style
>

> I really, really don�t care what this is. I�m thinking that we pick whatever people think that the current style is and give folks a fixed period to debate changes.

>
> 2. create some tools
>
> These tools should help people conform to the style.
>

> Primarily, what is needed is a tool with appropriate configuration that runs on the command line � e.g., mach reformat � clang-format is looking like a good candidate for C/C++, it just needs a configuration. For JavaScript, I�ve used the python js-beautify, but it�s pretty light on configuration options, it might need some hacking to make it better.
>
> Ideally though, there should be set of configuration files for common editors. I�m certain there are plenty out there already. Let�s bring them all together (attaching the files https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style might be enough).
>
> 3. reformat everything
>
> Take the command line tool and run it over all the code. I realise that this is the contentious part, but you don�t get the real benefits until you do this.
>
> Once you do this, it�s safe to reformat files at any time without messing with parts of files that you haven�t touched. This is important because many tools only reformat entire files.
>
> As for `hg blame` and related tools, I have a workaround for that. I�ve knocked together a tool that takes a file and a reformatter command line, and churns out a series of patches that retain blame: https://github.com/martinthomson/blame-bridge

>
> The patch bitrot problem is not easy to work around, but that will depend on how closely the affected files already conformed to the style guide.
>
> 4. enforce compliance
>
> This is probably a step for the future, but if there was - for example - a commit bot that waited for a clean build+test run, adding a format check to that run would allow the bot to block patches that screwed up the formatting of files.
>

> �Martin

[Ehsan Akhgari]

On 1/6/2014, 1:22 PM, Axel Hecht wrote:
> Hi,
>
> two points of caution:
>
> In the little version control archaeology I do, I hit "breaks blame for
> no good reason" pretty often already. I'd not underestimate the cost for
> the project of doing changes just for the sake of changes.

I have very rarely run into a blame session where you find what you're
looking for by just running blame once. Most of the time you need to
reblame on older revisions several times, and whitespace changes will
add at most one step if entire files are changed in single patches.
That being said, both hg and git support options to ignore whitespace
changes during a blame, so this issue is moot in practice (hgweb doesn't
support it, but it's a terrible blame tool anyway.)

> Tools don't get code right. I've seen various changes where tools
> reformat code they don't understand, and break it. Trailing whitespace
> is significant in some of our file formats, for example.

They're insignificant in C, C++, JS and Java.

Cheers,
Ehsan

[smaug]

On 01/06/2014 08:06 PM, Bobby Holley wrote:
> I think trying to change SpiderMonkey style to Gecko isn't a great use of
> effort:
> * There are a lot of SpiderMonkey hackers who don't hack on anything else,
> and don't consider themselves Gecko hackers. Many of them don't read
> dev-platform, and would probably revolt if this were to occur.
> * SpiderMonkey is kind of an external module, with an existing community of
> embedders who use its style.
> * SpiderMonkey has an incredibly detailed style guide - more detailed than
> Gecko's: https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style

I don't see that being any more detailed than Mozilla coding style.
Perhaps more verbal, but not more detailed.

[Adam Roach]

On 1/6/14 12:22, Axel Hecht wrote:
> In the little version control archaeology I do, I hit "breaks blame
> for no good reason" pretty often already. I'd not underestimate the
> cost for the project of doing changes just for the sake of changes.

Do you have a concrete reason to believe that Martin's workaround for
retaining blame information doesn't work, or did you just miss that part
of his message?


--
Adam Roach
Principal Platform Engineer
a...@mozilla.com
+1 650 903 0800 x863

[Mike Hoye]

I was asked, in the previous discussion about code formatting, to
provide some research on the subject. I'm sorry I didn't get back to
that fast enough, but here we are.

We have a lot of data on this: Michael Hansen at Indiana University has
done a series of eye-tracking experiments regarding code formatting
standards and programmer efficiency and comprehension.

He's written a blog post about it here:
http://synesthesiam.com/posts/what-makes-code-hard-to-understand.html

...and the longer publication is here:
http://arxiv.org/pdf/1304.5257v2.pdf - linked from
http://arxiv.org/abs/1304.5257 if you want citation information or other
formats.

You should read the whole thing, obviously, but a few choice
observations include the fact that small inconsistencies in formatting
have been unequivocally proven to cause dramatic drops in comprehension
and productivity. Likewise, dead code or even just code that doesn't
follow logically from the code above it.

Another observation is that experienced programmers just don't read code
the same way novices do. Novices go word-by-word, line-by-line, and
veterans tend to jump around scanning for keywords and code blocks; the
kicker is that when experienced software engineers see a file or code
block that's in an inconsistent _or just unfamiliar_ format, their
brains kick them out of their block-analysis mode and they have to go
back to reading line by line.

You can actually _see this happen_ with eye tracking software, is the
amazing thing.

The implication, relevant to individual employees and community
contributors as well as Mozilla as a fast-moving organization, is that:

- by not having a single coding standard we are making it a lot more
expensive and error-prone for contributors and employees to move from
one part of the project to another, and
- those costs are very real and largely hidden.

There's a long history of research - Solloway and Ehrlich as early as
1984 is one example - that says pretty much the same thing. Experienced
programmers have very strong, learned expectations about what a codebase
_should_ look like, and when it does not look like that, or when they
have to get used to a new system, productivity and comprehension drop
sharply.

Their paper's available here:
http://www.ics.uci.edu/~redmiles/inf233-FQ07/oldpapers/SollowayEhrlich.pdf
- but it's pretty typically hard-on-the-eyes early IEEE stuff, a scan of
a printout.

Coding standards are nice, and we should have them for sure, but they
should inform the design of a formatting tool, not be a style coach. We
should have a standard, and it should be enforced by a machine before
any of this ever hits a human - particularly a reviewer - in the eyes.
And from a community-engagement perspective, this matters. Every time we
type "nit" and send a patch back instead of cleaned up and ship-ready by
a tool is one more chance to miss an bunch of opportunities, to have
somebody get fed up and leave and let that patch rot on the vine. It's
one more small, annoying barrier to participation, and it'd be great if
we could have automation in place to make it go away.


- mhoye

On 1/6/2014, 12:55 PM, Martin Thomson wrote:
> I think that this is a good start, but it doesn�t go quite far enough.
>
> Part of the problem with a policy that requires people to avoid reformatting of stuff they don�t touch is that it propagates formatting divergence. Sometimes because it�s better to conform to the style immediately adjacent the changed, but more so because it prevents the use of tools that reformat entire files (here, I�m not talking about things like names, but more whitespace conventions).
>
> For whitespace at least, I think that we need to do the following:
>
> 1. pick a style
>
> I really, really don�t care what this is. I�m thinking that we pick whatever people think that the current style is and give folks a fixed period to debate changes.
>
> 2. create some tools
>
> These tools should help people conform to the style.
>
> Primarily, what is needed is a tool with appropriate configuration that runs on the command line � e.g., mach reformat � clang-format is looking like a good candidate for C/C++, it just needs a configuration. For JavaScript, I�ve used the python js-beautify, but it�s pretty light on configuration options, it might need some hacking to make it better.
>
> Ideally though, there should be set of configuration files for common editors. I�m certain there are plenty out there already. Let�s bring them all together (attaching the files https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style might be enough).
>
> 3. reformat everything
>
> Take the command line tool and run it over all the code. I realise that this is the contentious part, but you don�t get the real benefits until you do this.
>
> Once you do this, it�s safe to reformat files at any time without messing with parts of files that you haven�t touched. This is important because many tools only reformat entire files.
>
> As for `hg blame` and related tools, I have a workaround for that. I�ve knocked together a tool that takes a file and a reformatter command line, and churns out a series of patches that retain blame: https://github.com/martinthomson/blame-bridge
>
> The patch bitrot problem is not easy to work around, but that will depend on how closely the affected files already conformed to the style guide.
>
> 4. enforce compliance
>
> This is probably a step for the future, but if there was - for example - a commit bot that waited for a clean build+test run, adding a format check to that run would allow the bot to block patches that screwed up the formatting of files.
>
> �Martin
>
>

[Chris Peterson]

On 1/6/14, 9:55 AM, Martin Thomson wrote:
> 2. create some tools
>
> These tools should help people conform to the style.
>

> Primarily, what is needed is a tool with appropriate configuration that runs on the command line � e.g., mach reformat � clang-format is looking like a good candidate for C/C++, it just needs a configuration. For JavaScript, I�ve used the python js-beautify, but it�s pretty light on configuration options, it might need some hacking to make it better.
>
> Ideally though, there should be set of configuration files for common editors. I�m certain there are plenty out there already. Let�s bring them all together (attaching the fileshttps://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style might be enough).

That's a good idea. We codify our style guidelines and provide config
files for popular editors and reformatters (like clang-format, astyle,
and uncrustify) in the mozilla-central tree.


chris

[Karl Tomlinson]

Nicholas Nethercote writes:

> We've had some recent discussions about code style. I have a propasal

I'm sceptical about the value of such changes out-weighing the
cost here. I know there will be a cost now and in the future, but
I don't recall seeing any entire files so badly formatted that it
took me more time to understand the code. If there are any styles
in use that really do make the code harder to read then I would
support modifications to improve readability of that particular code.

However, if the differences are costing more than the costs of
changing, and this proposal goes ahead, can we try to limit the
cost of changing, please?

It's already been mentioned that some blame tools can already
handle whitespace changes. Similarly some patch tools can handle
whitespace changes. These features could help with the cost of
merging outstanding and in-progress patches if the kind of style
changes are limited appropriately. This would require that

1. Mass style changes consist only of changing sequences of one or
more blanks to another sequence of one or more blanks.

2. No code is moved to a different line. This actually follows
from 1, but is worth calling out separately because moving code
around would cause the biggest problems.

That might seem like a small subset of the differences in style,
but indentation differences are probably the biggest cost of moving
code from a file of one style to a file of another.

[Brian Smith]

On Sun, Jan 5, 2014 at 6:34 PM, Nicholas Nethercote
<n.neth...@gmail.com> wrote:
> - There is an semi-official policy that the owner of a module can dictate its
> style. Examples: SpiderMonkey, Storage, MFBT.

AFAICT, there are not many rules that module owners are bound by. The
reason module owners can dictate style is because module owners can
dictate everything in their module. I think we should wait until we've
heard from module owners that strongly oppose the style changes and
then decide how to deal with that. Imposing changes on module owners,
that the module owners don't agree to goes against the governance
system we have in place. Our governance system is based on the idea
that module owners (and peers) will make good decisions. Implicit in
that is the idea that module owners may need to make decisions that
are sub-optimal for them, but which are optimal for the project in
general.

> There appears to be no good reason for this and I propose we remove it.
> Possibly with the exception of SpiderMonkey (and XPConnect?), due to it being
> an old and large module with its own well-established style.

I guess you are implicitly excepting NSS and NSPR too, which are C code.

As far as PSM is concerned, my main ask is that such reformatting of
security/manager/ssl/src/* be done in February or later, so that the
current urgently-needed big refactoring in that code is not disrupted.

Cheers,
Brian
--
Mozilla Networking/Crypto/Security (Necko/NSS/PSM)

[Martin Thomson]

On 2014-01-06, at 12:36, Karl Tomlinson <moz...@karlt.net> wrote:

> I'm sceptical about the value of such changes out-weighing the
> cost here. I know there will be a cost now and in the future, but
> I don't recall seeing any entire files so badly formatted that it
> took me more time to understand the code. If there are any styles
> in use that really do make the code harder to read then I would
> support modifications to improve readability of that particular code.

I think that the mail from Mike Hoye provides reasonable evidence that good formatting is valuable, and more so than I first thought.

> However, if the differences are costing more than the costs of
> changing, and this proposal goes ahead, can we try to limit the
> cost of changing, please?
>
> It's already been mentioned that some blame tools can already
> handle whitespace changes. Similarly some patch tools can handle
> whitespace changes. These features could help with the cost of
> merging outstanding and in-progress patches if the kind of style
> changes are limited appropriately. This would require that
>
> 1. Mass style changes consist only of changing sequences of one or
> more blanks to another sequence of one or more blanks.

I would like to think that adding (or removing) braces from block statements should be acceptable.

> 2. No code is moved to a different line. This actually follows
> from 1, but is worth calling out separately because moving code
> around would cause the biggest problems.

I don’t think that this is a useful constraint. On the contrary, it hampers the formatting tools in ways that are detrimental to the overall end goal. If your intent is to be able to move toward a consistent style, that will require that the occasional character move between lines.

The tool that I posted a link to earlier has ways to maintain blame across both of the above sorts of changes.

> That might seem like a small subset of the differences in style,
> but indentation differences are probably the biggest cost of moving
> code from a file of one style to a file of another.

I disagree. I think that the bigger costs come from changing naming conventions (_memberVariable to memberVariable_ to mMemberVariable), because that can’t be done a file at a time.

[Martin Thomson]

On 2014-01-06, at 13:27, Brian Smith <br...@briansmith.org> wrote:

> As far as PSM is concerned, my main ask is that such reformatting of
> security/manager/ssl/src/* be done in February or later, so that the
> current urgently-needed big refactoring in that code is not disrupted.

For one, I don’t see this being done prior to February.

But more to the point, where there are major changes in play, I see no reason that a reformat should be simultaneous. If the tools are available, module owners can run them when is convenient for them.

[Robert O'Callahan]

On Tue, Jan 7, 2014 at 7:06 AM, Bobby Holley <bobby...@gmail.com> wrote:

> * There are a lot of SpiderMonkey hackers who don't hack on anything else,
> and don't consider themselves Gecko hackers.
>

This has always bothered me a lot. It needs to change.

Rob
--
Jtehsauts tshaei dS,o n" Wohfy Mdaon yhoaus eanuttehrotraiitny eovni
le atrhtohu gthot sf oirng iyvoeu rs ihnesa.r"t sS?o Whhei csha iids teoa
stiheer :p atroa lsyazye,d 'mYaonu,r "sGients uapr,e tfaokreg iyvoeunr,
'm aotr atnod sgaoy ,h o'mGee.t" uTph eann dt hwea lmka'n? gBoutt uIp
waanndt wyeonut thoo mken.o w

[Robert O'Callahan]

On Tue, Jan 7, 2014 at 10:27 AM, Brian Smith <br...@briansmith.org> wrote:

> AFAICT, there are not many rules that module owners are bound by.

There are lots of tree-wide or Gecko-wide rules that module owners are

bound by.


> The
> reason module owners can dictate style is because module owners can
> dictate everything in their module.

That's just not true, sorry. If some module owner decides to keep using
NULL or PRUnichar, or invent their own string class, they will be corrected.

I think we should wait until we've
> heard from module owners that strongly oppose the style changes and
> then decide how to deal with that.

We don't need to wait for them. When we discuss any broad issue that
affects their module, they are responsible for contributing to that
discussion in a timely manner if they want to have a say.

[Ehsan Akhgari]

FWIW I should mention explicitly that I support this proposal. The only
big thing that I wish to see changed here is to remove the exception for
js/* but I can live with that exception being lifted in the future.

Cheers,
Ehsan

On 1/5/2014, 9:34 PM, Nicholas Nethercote wrote:
> We've had some recent discussions about code style. I have a propasal
>

> - There is an semi-official policy that the owner of a module can dictate its
> style. Examples: SpiderMonkey, Storage, MFBT.
>

> There appears to be no good reason for this and I propose we remove it.
> Possibly with the exception of SpiderMonkey (and XPConnect?), due to it being
> an old and large module with its own well-established style.
>

[Jason Orendorff]

On 1/6/14 3:27 PM, Brian Smith wrote:
> On Sun, Jan 5, 2014 at 6:34 PM, Nicholas Nethercote
> <n.neth...@gmail.com> wrote:

>> - There is an semi-official policy that the owner of a module can dictate its
>> style. Examples: SpiderMonkey, Storage, MFBT.

> AFAICT, there are not many rules that module owners are bound by. The

> reason module owners can dictate style is because module owners can

> dictate everything in their module. I think we should wait until we've

> heard from module owners that strongly oppose the style changes and
> then decide how to deal with that.

I'm fine with changing SpiderMonkey in whatever way is necessary to stop
these threads forever.

-j

[Karl Tomlinson]

Martin Thomson writes:

> I would like to think that adding (or removing) braces from block statements
> should be acceptable.

I would argue that braces should not be added with automation.

When debugging code, it is important to understand the intent of
the author. Adding braces at parser-determined points disguises
the intent.

Actually, the same argument would prefer that code should not be
automatically re-indented to parser-determined tabs.

> The tool that I posted a link to earlier has ways to maintain blame across
> both of the above sorts of changes.

I get the impression that blame-bridge's goal is to attribute
lines to the correct person, but what we want is to identify the
changeset that made the code as it is now. I assume that can be
done if we accept another level of indirection.

This won't help in-progress patches, anyway.

> I disagree. I think that the bigger costs come from changing naming
> conventions (_memberVariable to memberVariable_ to mMemberVariable), because
> that can’t be done a file at a time.

We are talking about different things here. Yes, it will be
harder to automatically fix up patches for variable renaming
than for whitespace changes, but at least the compiler will
usually detect problems with different variable names.

_memberVariable to memberVariable_ to mMemberVariable is another
situation where the particular choice does not matter. All are
readable, so the only cost is when moving code from a file that
uses memberVariable_ to one that uses mMemberVariable. That is
not something we do too often.

[Daniel Holbert]

On 01/06/2014 03:10 PM, Karl Tomlinson wrote:
> Martin Thomson writes:
>
>> I would like to think that adding (or removing) braces from block statements
>> should be acceptable.
>
> I would argue that braces should not be added with automation.
>
> When debugging code, it is important to understand the intent of
> the author. Adding braces at parser-determined points disguises
> the intent.

I agree with karl. Automated insertion of braces could easily hide bugs.

e.g. consider this buggy code:
if (condition)
foo();
bar();

If we allow a tool to automatically place braces around "foo()", then
that won't change behavior, but it will hide the fact that the author
*meant* for both foo() and bar() to be inside the block.

[Joshua Cranmer 🐧]

On 1/6/2014 4:27 PM, Robert O'Callahan wrote:
> That's just not true, sorry. If some module owner decides to keep
> using NULL or PRUnichar, or invent their own string class, they will
> be corrected.

Maybe. But we also have a very large number of deprecated or
effectively-deprecated constructs whose deprecation module owners may
not be aware of because their use is somewhat prevalent in code. For
example, the NS_ENSURE_* macros are apparently now considered officially
deprecated. Our track record of removing these quickly is poor
(nsISupportsArray and nsVoidArray, anyone?), and many of the deprecated
constructs are macros or things defined in external project headers
(like, say, prtypes.h), which makes using __declspec(deprecated) or
__attribute__((deprecated)) unfeasible.

Is there any support for setting up a wiki page that lists these
deprecated, obsolete constructs and provides tracking bugs for actually
eliminating them?

[Ehsan Akhgari]

On 1/6/2014, 6:38 PM, Joshua Cranmer 🐧 wrote:
> On 1/6/2014 4:27 PM, Robert O'Callahan wrote:
>> That's just not true, sorry. If some module owner decides to keep
>> using NULL or PRUnichar, or invent their own string class, they will
>> be corrected.
>
> Maybe. But we also have a very large number of deprecated or
> effectively-deprecated constructs whose deprecation module owners may
> not be aware of because their use is somewhat prevalent in code. For
> example, the NS_ENSURE_* macros are apparently now considered officially
> deprecated. Our track record of removing these quickly is poor
> (nsISupportsArray and nsVoidArray, anyone?), and many of the deprecated
> constructs are macros or things defined in external project headers
> (like, say, prtypes.h), which makes using __declspec(deprecated) or
> __attribute__((deprecated)) unfeasible.

FWIW a while ago I got Wan-Teh's approval for us to take local Mozilla
specific patches to NSPR.

> Is there any support for setting up a wiki page that lists these
> deprecated, obsolete constructs and provides tracking bugs for actually
> eliminating them?

Yes please!

Cheers,
Ehsan

[smaug]

On 01/07/2014 01:38 AM, Joshua Cranmer 🐧 wrote:
> On 1/6/2014 4:27 PM, Robert O'Callahan wrote:
>> That's just not true, sorry. If some module owner decides to keep using NULL or PRUnichar, or invent their own string class, they will be corrected.
>
> Maybe. But we also have a very large number of deprecated or effectively-deprecated constructs whose deprecation module owners may not be aware of
> because their use is somewhat prevalent in code. For example, the NS_ENSURE_* macros are apparently now considered officially deprecated.

Since when? NS_ENSURE_ macros are very useful for debugging. (When something is going wrong, the warnings in the terminal tend to give strong
hints what/where that something is. Reduces debugging time significantly.)

[Gregory Szorc]

On 1/6/14, 3:35 PM, Daniel Holbert wrote:
> On 01/06/2014 03:10 PM, Karl Tomlinson wrote:

>> Martin Thomson writes:
>>
>>> I would like to think that adding (or removing) braces from block statements
>>> should be acceptable.
>>
>> I would argue that braces should not be added with automation.
>>
>> When debugging code, it is important to understand the intent of
>> the author. Adding braces at parser-determined points disguises
>> the intent.
>

> I agree with karl. Automated insertion of braces could easily hide bugs.
>
> e.g. consider this buggy code:
> if (condition)
> foo();
> bar();
>
> If we allow a tool to automatically place braces around "foo()", then
> that won't change behavior, but it will hide the fact that the author
> *meant* for both foo() and bar() to be inside the block.

I think you just gave an example of why our tooling needs to identify
poorly formatted code better. An "if" without a brace followed by
multiple indented lines containing compiler expressions is a bug at
worst or unreadable/unclear code at best. We could, of course, identify
such poor style before any automated style conversion. In any case,
looking at a file revision from before the automated brace insertion
would clearly reveal the author's [likely] intent. I therefore fail to
see your concern here.

[Jeff Walden]

On 01/06/2014 04:27 PM, Robert O'Callahan wrote:
> That's just not true, sorry. If some module owner decides to keep using
> NULL or PRUnichar, or invent their own string class, they will be corrected.

At least the former isn't true, as far as I remember. It took convincing to get JS to move over from NULL to nullptr; it was not something that could just be done, without any asking at all.

I'm not as sure about PRUnichar, just because there's so little strongly-owned code that everyone more or less always used PRUnichar.

The third, I would be astounded if we don't have minor string classes floating around somewhere in the tree.

Jeff

[Joshua Cranmer 🐧]

On 1/6/2014 6:06 PM, smaug wrote:
> On 01/07/2014 01:38 AM, Joshua Cranmer 🐧 wrote:
>> On 1/6/2014 4:27 PM, Robert O'Callahan wrote:
>>> That's just not true, sorry. If some module owner decides to keep
>>> using NULL or PRUnichar, or invent their own string class, they will
>>> be corrected.
>>
>> Maybe. But we also have a very large number of deprecated or
>> effectively-deprecated constructs whose deprecation module owners may
>> not be aware of
>> because their use is somewhat prevalent in code. For example, the
>> NS_ENSURE_* macros are apparently now considered officially deprecated.
> Since when? NS_ENSURE_ macros are very useful for debugging. (When
> something is going wrong, the warnings in the terminal tend to give
> strong
> hints what/where that something is. Reduces debugging time
> significantly.)

Since Benjamin's message of November 22:
<news://news.mozilla.org/mailman.11861.1385151...@lists.mozilla.org>
(search for "Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS" if you
don't have an NNTP client). Although there wasn't any prior discussion
of the wisdom of this change, whether or not to use
NS_ENSURE_SUCCESS-like patterns has been the subject of very acrimonious
debates in the past and given the voluminous discussion on style guides
in recent times, I'm not particularly inclined to start yet another one.

[Jeff Walden]

On 01/06/2014 04:46 PM, Jason Orendorff wrote:
> I'm fine with changing SpiderMonkey in whatever way is necessary to stop
> these threads forever.

Pretty sure I've said the same thing in other places, that we should have something everyone hates and use it everywhere. Definitely I've said it to people in real life. I am fine with everyone on the same page, here and in mfbt and everywhere else.

Which is not to say I like the current Mozilla style. It's largely tolerable. There are a few things that find pretty irksome, tho. I'll start a new thread for those things.

Jeff

[Patrick McManus]

I'm fine with enforcing a gecko wide coding style as long as it comes with
cross platform tools to act as arbiter.. it is something that needs to be
automated and isn't worth the effort of trying to get everybody on the same
page by best effort.

>> - There is an semi-official policy that the owner of a module can dictate
>> its
>> style. Examples: SpiderMonkey, Storage, MFBT.
>>

[Karl Tomlinson]

Gregory Szorc writes:

> I think you just gave an example of why our tooling needs to
> identify poorly formatted code better. An "if" without a brace
> followed by multiple indented lines containing compiler
> expressions is a bug at worst or unreadable/unclear code at
> best. We could, of course, identify such poor style before any
> automated style conversion. In any case, looking at a file
> revision from before the automated brace insertion would clearly
> reveal the author's [likely] intent. I therefore fail to see your
> concern here.

Consider

if (condition1)
if (condition2)
foo();
else
bar();

Automated style conversion would make this

if (condition1) {
if (condition2) {
foo();
} else {
bar();
}
}

No one is likely to look at the file revision before the style
conversion, but someone might look through the trunk revision.
The bug is initially visible but well hidden by the conversion.

The bug is hidden almost as well by conversion of only indentation.

if (condition1)
if (condition2)
foo();
else
bar();

Sure, you could argue that style conversion makes the actual
behaviour clearer, but you'd have to know the intended behaviour
to know there was a bug.

If we have a tool to skip the style change on any such unclear
situations, then perhaps we can proceed more safely.

[Gregory Szorc]

The clang formatter accepts lines, columns, or byte offsets to reformat. If we can identify nebulous blocks, we can construct arguments to ignore them. Better yet, we teach the clang formatter how to recognize nebulous scenarios (if it doesn't already).

[Chris Peterson]

On 1/6/14, 5:05 PM, Karl Tomlinson wrote:
> If we have a tool to skip the style change on any such unclear
> situations, then perhaps we can proceed more safely.

I would replace "skip" with "abort loudly", so a human can review the
unclear code. :)


chris

[Ms2ger]

FWIW, I've never seen much support for this change from anyone else than
Benjamin, and only in his modules the NS_ENSURE_* macros have been
effectively deprecated.

HTH
Ms2ger

[L. David Baron]

I'm happy about getting rid of NS_ENSURE_*.

-David

--
𝄞 L. David Baron http://dbaron.org/ 𝄂
𝄢 Mozilla https://www.mozilla.org/ 𝄂
Before I built a wall I'd ask to know
What I was walling in or walling out,
And to whom I was like to give offense.
- Robert Frost, Mending Wall (1914)

[Mike Hoye]

On 1/6/2014, 8:05 PM, Karl Tomlinson wrote:
> The bug is hidden almost as well by conversion of only indentation.
>
> if (condition1)
> if (condition2)
> foo();
> else
> bar();
>
> Sure, you could argue that style conversion makes the actual
> behaviour clearer, but you'd have to know the intended behaviour
> to know there was a bug.
>
> If we have a tool to skip the style change on any such unclear
> situations, then perhaps we can proceed more safely.

A syntactic change that's small, easy to review and limited to one part
of one file, you say?

So as you describe it, a tool that backs off and flags ambiguities like
this for human review would also be a tool that generates an _excellent_
set of Good First Bugs.


- mhoye

[Ehsan Akhgari]

On 1/6/2014, 8:05 PM, Karl Tomlinson wrote:

clang-format does not yet support automatic brace insertion. Once that
support is added, we can protect against these cases.

Cheers,
Ehsan

[Martin Thomson]

On 2014-01-06, at 17:05, Karl Tomlinson <moz...@karlt.net> wrote:

> if (condition1)
> if (condition2)
> foo();
> else
> bar();
>
> Sure, you could argue that style conversion makes the actual
> behaviour clearer, but you'd have to know the intended behaviour
> to know there was a bug.

I’ll just point out that this isn’t a bug if there are test cases that pass.

[Boris Zbarsky]

On 1/7/14 12:47 PM, Martin Thomson wrote:
> I�ll just point out that this isn�t a bug if there are test cases that pass.

That depends on whether the test cases are correct (e.g. whether they
were written independently of the code, whether they were reviewed, etc)....

-Boris

[Benoit Jacob]

2014/1/7 L. David Baron <dba...@dbaron.org>

> On Tuesday 2014-01-07 09:13 +0100, Ms2ger wrote:

> I'm happy about getting rid of NS_ENSURE_*.
>

I would like a random voice in favor of deprecating NS_ENSURE_* for the
reason that hiding control flow behind macros is IMO one of the most evil
usage patterns of macros.

Benoit

>
> -David
>
> --
> 𝄞 L. David Baron http://dbaron.org/ 𝄂
> 𝄢 Mozilla https://www.mozilla.org/ 𝄂
> Before I built a wall I'd ask to know
> What I was walling in or walling out,
> And to whom I was like to give offense.
> - Robert Frost, Mending Wall (1914)
>

[Gavin Sharp]

I support getting rid of NS_ENSURE_*.

Gavin

[Neil]

Benoit Jacob wrote:

>I would like a random voice in favor of deprecating NS_ENSURE_* for the reason that hiding control flow behind macros is IMO one of the most evil usage patterns of macros.
>

So you're saying that

nsresult rv = Foo();
NS_ENSURE_SUCCESS(rv, rv);

is hiding the control flow of the equivalent JavaScript

try {
Foo();
} catch (e) {
throw e;
}

except of course that nobody writes JavaScript like that...

--
Warning: May contain traces of nuts.

[Benoit Jacob]

2014/1/7 Neil <ne...@parkwaycc.co.uk>

All I mean is that NS_ENSURE_SUCCESS hides a 'return' statement.

#define NS_ENSURE_SUCCESS(res, ret)
\ do {
\ nsresult __rv = res; /* Don't evaluate |res| more than
once */ \ if (NS_FAILED(__rv)) {
\ NS_ENSURE_SUCCESS_BODY(res, ret)
\ return ret;
\ }
\ } while(0)


For example, if I'm scanning a function for possible early returns (say I'm
debugging a bug where we're forgetting to close or delete a thing before
returning), I now need to scan for NS_ENSURE_SUCCESS in addition to
scanning for return. That's why hiding control flow in macros is, in my
opinion, never acceptable.

Benoit

>
> --
> Warning: May contain traces of nuts.
>

[Joshua Cranmer 🐧]

On 1/7/2014 12:58 PM, Benoit Jacob wrote:
> I would like a random voice in favor of deprecating NS_ENSURE_* for
> the reason that hiding control flow behind macros is IMO one of the
> most evil usage patterns of macros.

In general, I would agree. I have no problems with killing, say,
NS_ENSURE_TRUE or similar macros. However, NS_ENSURE_SUCCESS is a
special case: it is the equivalent in JS of not providing a try/catch
statement around a JS expression. In code that makes extremely heavy use
of XPCOM requirements, NS_ENSURE_SUCCESS brings the amount of
boilerplate needed to one line per function; to not use it requires
significantly more boilerplate (three lines, if you insist on following
the always-brace-ifs convention).

[Benjamin Smedberg]

On 1/5/2014 9:34 PM, Nicholas Nethercote wrote:


>
> I propose that we officially remove this implicit discouragement, and even
> encourage changes that convert non-Mozilla-style code to Mozilla-style (with
> some exceptions; see below). When modifying badly-styled code, following
> existing style is still probably best.

I don't think that style changes are currently actively forbidden, but
they aren't exactly encouraged. What you're primarily changing is
whether they are encouraged.

I think that there are two important rules here:

* Real patches shouldn't mess with coding style
* Style changes should be coordinated with the module owner to minimize
conflicts with other patches that are in progress or planned.

Karl suggests in the thread that we should discourage reformatting which
moves code to a different line, which would appear to include braces. I
disagree, since bracing changes and whitespace formatting are the most
important issues for scanning code, and we should fix them all at once.

What's not clear to me from the discussion is whether there is a
reformatting tool which already exists which can do what we need. If
there is, I think we should actively discourage people reformatting by
hand, and just encourage people to use the tool at an appropriate time.

>
>
> - There is an semi-official policy that the owner of a module can dictate its
> style. Examples: SpiderMonkey, Storage, MFBT.

Spidermonkey is hard because it uses a different member naming
convention and so the changes are potentially much more invasive. But I
at least support starting out by unifying the indentation and bracing
styles across the tree, and removing other exceptions.

>
> Also, we probably shouldn't change the style of imported third-party code;

Yes.

I am the owner of at least the C/C++ portions of the style guide; I
propose to wait and see whether the C++ reformatting tools are of
sufficient quality that we can use them directly, to avoid
hand-reformatting, and make a final decision next week sometime.

In the meantime, we should wrap up the pending discussions about other
changes to the style guide, such as 80/100/infinite columns,
member/parameter/local naming convention, and other threads that Gavin
was going to start and one I'm going to start now ;-)

--BDS

[Chris Peterson]

On 1/7/14, 12:31 PM, Benjamin Smedberg wrote:
> What's not clear to me from the discussion is whether there is a
> reformatting tool which already exists which can do what we need. If
> there is, I think we should actively discourage people reformatting by
> hand, and just encourage people to use the tool at an appropriate time.

The "astyle" and "uncrustify" reformatting tools have options to add braces.


http://astyle.sourceforge.net/
http://uncrustify.sourceforge.net/


chris

[Ehsan Akhgari]

I'm also planning to see what it would take to teach clang-format about
braces...

Cheers,
Ehsan

[Nicholas Nethercote]

On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg
<benj...@smedbergs.us> wrote:
>
> I am the owner of at least the C/C++ portions of the style guide; I propose
> to wait and see whether the C++ reformatting tools are of sufficient quality
> that we can use them directly, to avoid hand-reformatting, and make a final
> decision next week sometime.

Thanks! Who will evaluate these tools?

BTW, jcranmer's point above about there being three categories of
rules (formatting, naming, other) is a good one. Tools can only help
with the first category (and possibly not even all rules within that
category). So some hand-reformatting will be inevitable.

Nick

[Neil]

Benoit Jacob wrote:

>I'm scanning a function for possible early returns
>

Good thing you don't have to deal with C++ exceptions then.

[Mike Hommey]

There are actually clang-based tools for e.g. variables renaming.

Mike

[Nicholas Nethercote]

On Tue, Jan 7, 2014 at 4:38 PM, Mike Hommey <m...@glandium.org> wrote:
> So some hand-reformatting will be inevitable.
>
> There are actually clang-based tools for e.g. variables renaming.

Thanks for the nit. I believe the broader point -- that tools won't
fix every style issue, so some hand-reformatting will be necessary --
still stands.

Nick

[Kyle Huey]

On Tue, Jan 7, 2014 at 11:29 AM, Benoit Jacob <jacob.b...@gmail.com>wrote:

> For example, if I'm scanning a function for possible early returns (say I'm
> debugging a bug where we're forgetting to close or delete a thing before
> returning), I now need to scan for NS_ENSURE_SUCCESS in addition to
> scanning for return. That's why hiding control flow in macros is, in my
> opinion, never acceptable.
>

If you care about that 9 times out of 10 you are failing to use an RAII
class when you should be.

Since we seem to be voting now, I am moderately opposed to making XPCOM
method calls more boilerplate-y, and very opposed to removing
NS_ENSURE_SUCCESS without some sort of easy shorthand to test an nsresult
and print to the console if it is a failure. I know for sure that some of
the other DOM peers (smaug and bz come to mind) feel similarly about the
latter.

- Kyle

[Mike Habicher]

Would a macro starting with RETURN_ be an improvement over NS_ENSURE_?

e.g.
nsresult rv = Foo();
RETURN_AND_WARN_IF_FAIL(rv);

It's a mouthful (handful?) to type, but it's a single line and makes
explicit what's going on.

--m.

[Bobby Holley]

On Tue, Jan 7, 2014 at 5:04 PM, Kyle Huey <m...@kylehuey.com> wrote:

> Since we seem to be voting now, I am moderately opposed to making XPCOM
> method calls more boilerplate-y, and very opposed to removing
> NS_ENSURE_SUCCESS without some sort of easy shorthand to test an nsresult
> and print to the console if it is a failure. I know for sure that some of
> the other DOM peers (smaug and bz come to mind) feel similarly about the
> latter.
>

I strongly second this. The harder you make it to check (and warn) for
exception conditions, the less people are going to end up doing it. And
losing the ability to trace the origin of an erroneous condition by
scanning the console log is a huge problem.

bholley

[Benoit Jacob]

2014/1/7 Kyle Huey <m...@kylehuey.com>

> On Tue, Jan 7, 2014 at 11:29 AM, Benoit Jacob <jacob.b...@gmail.com>wrote:
>
>> For example, if I'm scanning a function for possible early returns (say
>> I'm
>> debugging a bug where we're forgetting to close or delete a thing before
>> returning), I now need to scan for NS_ENSURE_SUCCESS in addition to
>> scanning for return. That's why hiding control flow in macros is, in my
>> opinion, never acceptable.
>>
>
> If you care about that 9 times out of 10 you are failing to use an RAII
> class when you should be.
>

I was talking about reading code, not writing code. I spend more time
reading code that I didn't write, than writing code. Of course I do use
RAII helpers when I write this kind of code myself, in fact just today I
landed two more such helpers in gfx/gl/ScopedGLHelpers.* ...

Benoit

>
> Since we seem to be voting now, I am moderately opposed to making XPCOM
> method calls more boilerplate-y, and very opposed to removing
> NS_ENSURE_SUCCESS without some sort of easy shorthand to test an nsresult
> and print to the console if it is a failure. I know for sure that some of
> the other DOM peers (smaug and bz come to mind) feel similarly about the
> latter.
>

> - Kyle
>

[Benjamin Smedberg]

On 1/7/2014 8:04 PM, Kyle Huey wrote:
>
> Since we seem to be voting now, I am moderately opposed to making XPCOM
> method calls more boilerplate-y, and very opposed to removing
> NS_ENSURE_SUCCESS without some sort of easy shorthand to test an nsresult
> and print to the console if it is a failure.

We aren't voting any more. This has been discussed at length over the
years, and the decision to use if (NS_WARN_IF()) is pretty much final.

The only outstanding question is whether we should have a form

if (NS_WARN_IF_FAILED(rv))

Or whether we should simply require

NS_WARN_IF(NS_FAILED(rv))

I am accepting input on this question in the other thread about it.
Right now I'm leaning toward requiring the longer form.

--BDS

[Nicholas Nethercote]

On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg
<benj...@smedbergs.us> wrote:
>
> I am the owner of at least the C/C++ portions of the style guide; I propose
> to wait and see whether the C++ reformatting tools are of sufficient quality
> that we can use them directly, to avoid hand-reformatting, and make a final
> decision next week sometime.
>

> In the meantime, we should wrap up the pending discussions about other
> changes to the style guide, such as 80/100/infinite columns,
> member/parameter/local naming convention, and other threads that Gavin was
> going to start and one I'm going to start now ;-)

It's been a couple of weeks now. Can we move forward?

Anthony Jones has done some work on using clang-format to restyle
lines that have changed in a file, but I don't think anyone has done
any evaluation of clang-format for whole-file changes.

I wonder if we could allow hand-reformatting (e.g.
https://bugzilla.mozilla.org/show_bug.cgi?id=956199), at least for the
moment? Because if we don't allow that, we're stuck in limbo until we
decide on official guidelines for using automatic tools.

Nick

[Henri Sivonen]

On Tue, Jan 7, 2014 at 2:42 AM, Patrick McManus <mcm...@ducksong.com> wrote:
> I'm fine with enforcing a gecko wide coding style as long as it comes with
> cross platform tools to act as arbiter.. it is something that needs to be
> automated and isn't worth the effort of trying to get everybody on the same
> page by best effort.

Yes.

It's very annoying that our (well, at least "our" in the DOM-ish parts
of the code base) current line wrapping conventions don't come
naturally from Eclipse CDT, for example, so I end up doing a lot of
formatting manually even though formatting should be something that
the IDE does.

If we put a lot of effort into standardizing code style, we should
use a style that can be taught to Eclipse CDT, XCode, VisualStudio,
Emacs, vim, etc. even if that means that we have to make the rules
less refined.

On Mon, Jan 6, 2014 at 6:07 PM, Adam Roach <a...@mozilla.com> wrote:
> * Don't reformat third-party code.

* Don't reformat generated code. (If you must, change the generator
accordingly or add an automated post-processor.)

* Don't enforce C++ style on non-C++ code. (I don't work on the Fennec
front end, so maybe I should not care, but it saddens me to see naming
conventions that go against the Sun conventions / defaults of every
Java IDE there.)

--
Henri Sivonen
hsiv...@hsivonen.fi
https://hsivonen.fi/

[Anthony Jones]

On 28/01/14 13:08, Nicholas Nethercote wrote:
> On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg
> <benj...@smedbergs.us> wrote:
>>
>> I am the owner of at least the C/C++ portions of the style guide; I propose
>> to wait and see whether the C++ reformatting tools are of sufficient quality
>> that we can use them directly, to avoid hand-reformatting, and make a final
>> decision next week sometime.

In my opinion clang-format is good enough.

>> In the meantime, we should wrap up the pending discussions about other
>> changes to the style guide, such as 80/100/infinite columns,
>> member/parameter/local naming convention, and other threads that Gavin was
>> going to start and one I'm going to start now ;-)
>
> It's been a couple of weeks now. Can we move forward?

A style verifier would be nice.

> Anthony Jones has done some work on using clang-format to restyle
> lines that have changed in a file, but I don't think anyone has done
> any evaluation of clang-format for whole-file changes.

I haven't received any feedback either positive or negative in relation
to clang-format. That makes it hard for me to know whether to continue
putting any effort into improving it.

I've tested the whole file reformatting and it seems reasonable to me.
There are some cases it doesn't know anything about like the indenting
associated with macros such as NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN and
NS_IMPL_CYCLE_COLLECTION_UNLINK_END.

There is also some vagueness and inconsistency around:

bool
MyClass::MyFunction()

vs

bool MyClass::MyFunction()

Most of the code seems to do this only for top-level functions i.e. when
not already indented. I'd like some clarification on the rule here. It
seems to be applied differently in different parts of the code.

> I wonder if we could allow hand-reformatting (e.g.
> https://bugzilla.mozilla.org/show_bug.cgi?id=956199), at least for the
> moment? Because if we don't allow that, we're stuck in limbo until we
> decide on official guidelines for using automatic tools.

Right now with my modified clang-format-3.5 you can do:
$ ~/.mozbuild/clang-format-3.5 -i --style=Mozilla MyFile.cpp

Can we make bzexport automatically tag patches that have gone through
clang-format?

Anthony

[Bobby Holley]

On Tue, Jan 28, 2014 at 7:15 PM, Anthony Jones <ajo...@mozilla.com> wrote:

> In my opinion clang-format is good enough.
>

That's awesome!

> I haven't received any feedback either positive or negative in relation
> to clang-format. That makes it hard for me to know whether to continue
> putting any effort into improving it.
>

I think this is important, and definitely it to get the tool right.

> I've tested the whole file reformatting and it seems reasonable to me.
> There are some cases it doesn't know anything about like the indenting
> associated with macros such as NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN and
> NS_IMPL_CYCLE_COLLECTION_UNLINK_END.
>

Do we have a path to fix it?

There is also some vagueness and inconsistency around:
>
> bool
> MyClass::MyFunction()
>
> vs
>
> bool MyClass::MyFunction()
>

I'm pretty sure the former is correct per Gecko style. Benjamin can confirm.

[Neil]

My understanding is that it helps when you diff -p (which is one of our
preferred diff flags) as you get as much of the function name as
possible in the diff.

[Benjamin Smedberg]

On 1/27/2014 7:08 PM, Nicholas Nethercote wrote:
>
> It's been a couple of weeks now. Can we move forward?
>

> Anthony Jones has done some work on using clang-format to restyle
> lines that have changed in a file, but I don't think anyone has done
> any evaluation of clang-format for whole-file changes.
>

> I wonder if we could allow hand-reformatting (e.g.
> https://bugzilla.mozilla.org/show_bug.cgi?id=956199), at least for the
> moment? Because if we don't allow that, we're stuck in limbo until we
> decide on official guidelines for using automatic tools.

Modules owners can *allow* hand reformatting to the style guide any time
they want. I don't want to encourage that in the style guide, though.

I would encourage people, rather than doing hand reformatting, to run
the clang-format tool on an entire file and identify with the module
owner whether it produced useful results. There are still some cases
that will need to be fixed by hand after (the CC macros especially).
That's the only way we're going to get feedback on clang-format anyway,
right?

--BDS

[Gregory Szorc]

On 1/28/14, 7:15 PM, Anthony Jones wrote:
> On 28/01/14 13:08, Nicholas Nethercote wrote:
>>> In the meantime, we should wrap up the pending discussions about other
>>> changes to the style guide, such as 80/100/infinite columns,
>>> member/parameter/local naming convention, and other threads that Gavin was
>>> going to start and one I'm going to start now ;-)
>>
>> It's been a couple of weeks now. Can we move forward?
>
> A style verifier would be nice.

Building a style verifier for C++ is hard. You inevitably have to build
it on top of an existing parser (like Clang) or cobble something hacky
together [1], at which point you are chasing the language evolution and
all kinds of one-offs.

I think it would be much easier to have the verifier be a derivative of
the formatting tool. Ideally the formatter would emit reasons/policies
why it is changing something. In read-only mode, it would behave as a
verifier. The Clang project might be receptive to this enhancement if it
were proposed to them.

[1] http://google-styleguide.googlecode.com/svn/trunk/cpplint/cpplint.py

[Karl Tomlinson]

Anthony Jones writes:

> There is also some vagueness and inconsistency around:
>
> bool
> MyClass::MyFunction()
>
> vs
>
> bool MyClass::MyFunction()
>

> Most of the code seems to do this only for top-level functions i.e. when
> not already indented. I'd like some clarification on the rule here. It
> seems to be applied differently in different parts of the code.

Neil <ne...@parkwaycc.co.uk> writes:

> Bobby Holley wrote:
>
>>I'm pretty sure the former is correct per Gecko style. Benjamin can confirm.
>>
>>
> My understanding is that it helps when you diff -p (which is one
> of our preferred diff flags) as you get as much of the function
> name as possible in the diff.

It helps diff -p when the function is not indented.

When the function is indented, such as inline functions in a class
declaration, diff -p doesn't know where the function starts, so it
tells you whether the block is public, private, etc.

[Anthony Jones]

On 30/01/14 06:32, Gregory Szorc wrote:
> Building a style verifier for C++ is hard. You inevitably have to build
> it on top of an existing parser (like Clang) or cobble something hacky
> together [1], at which point you are chasing the language evolution and
> all kinds of one-offs.

I wrote one using the Haskell Language.C library which parses
preprocessed output but I no longer have the code. Someone who can
actually program in Haskell could do it very easily. You just do some
simple pattern matching on the AST.

The hard part (at least for me) would be the build system changes to get
the preprocessed output.

> I think it would be much easier to have the verifier be a derivative of
> the formatting tool. Ideally the formatter would emit reasons/policies
> why it is changing something. In read-only mode, it would behave as a
> verifier. The Clang project might be receptive to this enhancement if it
> were proposed to them.

I haven't looked hard enough at the clang-format code to work out how
hard it would be. Eclipse CDT has support for refactoring including
changing variable names. It doesn't work spectacularly well and I expect
that is because it is actually a hard problem. It is also partly because
Eclipse doesn't have all the information that our build system currently
has.

I don't think we should attempt style rewriting.

One thing I wanted to explain is why I have focussed on
clang-format-diff more than clang-format. My approach is to stop
introducing ugliness so that we ratchet towards consistency. I suggest
we do the same for style. The advantage of this approach is that it
allows us to improve the tools as we go and avoids a flag day.

Anthony

[Bobby Holley]

On Wed, Jan 29, 2014 at 1:21 PM, Anthony Jones <ajo...@mozilla.com> wrote:

> I don't think we should attempt style rewriting.
>
> One thing I wanted to explain is why I have focussed on
> clang-format-diff more than clang-format. My approach is to stop
> introducing ugliness so that we ratchet towards consistency. I suggest
> we do the same for style. The advantage of this approach is that it
> allows us to improve the tools as we go and avoids a flag day.
>

This tool is not going to be very helpful in a lot of modules until we do
style-rewriting. I'm not about to start taking piecemeal 2-space-indented
patches in XPConnect.

bholley

[Kyle Huey]

On Wed, Jan 29, 2014 at 9:32 AM, Gregory Szorc <g...@mozilla.com> wrote:
> On 1/28/14, 7:15 PM, Anthony Jones wrote:
>>

>> On 28/01/14 13:08, Nicholas Nethercote wrote:
>>>>
>>>> In the meantime, we should wrap up the pending discussions about other
>>>> changes to the style guide, such as 80/100/infinite columns,
>>>> member/parameter/local naming convention, and other threads that Gavin
>>>> was
>>>> going to start and one I'm going to start now ;-)
>>>
>>>
>>> It's been a couple of weeks now. Can we move forward?
>>
>>
>> A style verifier would be nice.
>
>

> Building a style verifier for C++ is hard. You inevitably have to build it
> on top of an existing parser (like Clang) or cobble something hacky together
> [1], at which point you are chasing the language evolution and all kinds of
> one-offs.
>

> I think it would be much easier to have the verifier be a derivative of the
> formatting tool. Ideally the formatter would emit reasons/policies why it is
> changing something. In read-only mode, it would behave as a verifier. The
> Clang project might be receptive to this enhancement if it were proposed to
> them.
>

> [1] http://google-styleguide.googlecode.com/svn/trunk/cpplint/cpplint.py

>
> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

Whatever tool we end up using needs to work on Windows.

- Kyle

[Zack Weinberg]

On 2014-01-29 4:21 PM, Anthony Jones wrote:
>
> I don't think we should attempt style rewriting.
>
> One thing I wanted to explain is why I have focussed on
> clang-format-diff more than clang-format. My approach is to stop
> introducing ugliness so that we ratchet towards consistency. I suggest
> we do the same for style. The advantage of this approach is that it
> allows us to improve the tools as we go and avoids a flag day.

On a number of style points - most importantly indentation depth -
consistency within a single file is *far* more important than
conformance with a global standard. If we're not prepared to reformat
to the new style at least on a file-by-file basis, I don't think we
should be trying to enforce global consistency wrt some of these things
at all.

zw

[Sylvestre Ledru]

On 29/01/2014 13:46, Kyle Huey wrote:
> On Wed, Jan 29, 2014 at 9:32 AM, Gregory Szorc <g...@mozilla.com> wrote:
>> On 1/28/14, 7:15 PM, Anthony Jones wrote:

>>> On 28/01/14 13:08, Nicholas Nethercote wrote:
>>>>> In the meantime, we should wrap up the pending discussions about other
>>>>> changes to the style guide, such as 80/100/infinite columns,
>>>>> member/parameter/local naming convention, and other threads that Gavin
>>>>> was
>>>>> going to start and one I'm going to start now ;-)
>>>>
>>>> It's been a couple of weeks now. Can we move forward?
>>>
>>> A style verifier would be nice.

>> [...]

> Whatever tool we end up using needs to work on Windows.

LLVM tools (like clang-format) are now supported under Windows (
http://llvm.org/builds/ ), including clang-format.
If they are not working, you can consider that as a bug.

Sylvestre

[Anthony Jones]

On 30/01/14 10:46, Kyle Huey wrote:
> Whatever tool we end up using needs to work on Windows.

clang-format works fine on Windows.

If we want to go down the preprocessed route, you can generate
preprocessed output from the msvc compiler using:

C:\> cl /E MyFile.cpp

Pretty much does the same thing as `gcc -E` or `clang -E`.

Anthony

[Anthony Jones]

Having looked into it some more I've concluded that libclang is probably
the way to go.

Anthony

[Anthony Jones]

On 30/01/14 10:42, Bobby Holley wrote:
> On Wed, Jan 29, 2014 at 1:21 PM, Anthony Jones <ajo...@mozilla.com

> <mailto:ajo...@mozilla.com>> wrote:
>
> I don't think we should attempt style rewriting.
>
> One thing I wanted to explain is why I have focussed on
> clang-format-diff more than clang-format. My approach is to stop
> introducing ugliness so that we ratchet towards consistency. I suggest
> we do the same for style. The advantage of this approach is that it
> allows us to improve the tools as we go and avoids a flag day.
>
>

> This tool is not going to be very helpful in a lot of modules until we
> do style-rewriting. I'm not about to start taking piecemeal
> 2-space-indented patches in XPConnect.

When I said "style rewriting" I was referring to changing code not
whitespace.

Anthony

[Nicholas Nethercote]

On Thu, Jan 30, 2014 at 11:42 AM, Anthony Jones <ajo...@mozilla.com> wrote:
>>
>> This tool is not going to be very helpful in a lot of modules until we
>> do style-rewriting. I'm not about to start taking piecemeal
>> 2-space-indented patches in XPConnect.
>
> When I said "style rewriting" I was referring to changing code not
> whitespace.

Indentation depth is an element of code style; indeed, it's one of the
most important ones because it affects most lines in a file.

One of my primary goals with starting this discussion was to reach
agreement that mass style fixes (e.g. covering entire files) are
acceptable. Because the current "only fix style when you change lines"
approach means many old files are forever stuck in an old style.

I think what Bobby is saying is that a tool which restyles only lines
that have only been modified isn't much use. For example, much of
XPConnect uses 4-space indents, when it should use 2-space indents,
and fixing that cannot be sensibly done in any way other than an
entire file (or files) at a time. (And if we're going to fix
indentation, obviously we should fix any other style problems at the
same time.)

In contrast, a tool that can correctly restyle entire files *is* useful.

Nick

[Benjamin Smedberg]

On 1/30/2014 7:25 PM, Nicholas Nethercote wrote:
> One of my primary goals with starting this discussion was to reach
> agreement that mass style fixes (e.g. covering entire files) are
> acceptable.

I agree. We should focus our efforts on converting whole files of code
to our new style and then enforcing consistency.

> entire file (or files) at a time. (And if we're going to fix
> indentation, obviously we should fix any other style problems at the
> same time.)

I'm not sure that we need to fix all the problems in order to be useful.
I'd certainly automatically-generated whole-file patches which just do
re-indenting and some simple brace fixup.

--BDS

[Anthony Jones]

On 31/01/14 13:25, Nicholas Nethercote wrote:
> I think what Bobby is saying is that a tool which restyles only lines
> that have only been modified isn't much use. For example, much of
> XPConnect uses 4-space indents, when it should use 2-space indents,
> and fixing that cannot be sensibly done in any way other than an
> entire file (or files) at a time. (And if we're going to fix
> indentation, obviously we should fix any other style problems at the
> same time.)

In order to make this happen I've run clang-format on XPConnect and
uploaded it to bug 966840.

Anthony

[Brian Smith]

I was curious about how this might eventually affect the stuff I work
on, so I looked at a couple of files' changes in the patch. I found
several omissions of advice from the style guide; the style guide
should be expanded to specify what the recommended style is in more
cases. See my comments in the bug.

Cheers,
Brian
--
Mozilla Networking/Crypto/Security (Necko/NSS/PSM)

[Bobby Holley]

On Sun, Feb 2, 2014 at 5:01 PM, Anthony Jones <ajo...@mozilla.com> wrote:

XPConnect currently follows JS-style, which is the most divergent style in
the tree (in particular, 4-space indents mean that a restyle is going to
rewrite every line). As such, I don't think it's a great place to prototype
clang-format restyles. At minimum, we should do dom/ and content/ first.

bholley

[Nicholas Nethercote]

On Sun, Feb 2, 2014 at 5:38 PM, Bobby Holley <bobby...@gmail.com> wrote:
>
> XPConnect currently follows JS-style, which is the most divergent style in
> the tree (in particular, 4-space indents mean that a restyle is going to
> rewrite every line). As such, I don't think it's a great place to prototype
> clang-format restyles. At minimum, we should do dom/ and content/ first.

xpcom/base/nsMemoryReporterManager.{h,cpp} would be an interesting
test, since I have handwritten patches to fix their style in
https://bugzilla.mozilla.org/show_bug.cgi?id=956199.

Nick

[Nicholas Nethercote]

On Fri, Jan 31, 2014 at 6:08 AM, Benjamin Smedberg
<benj...@smedbergs.us> wrote:
>
> I'm not sure that we need to fix all the problems in order to be useful. I'd
> certainly automatically-generated whole-file patches which just do
> re-indenting and some simple brace fixup.

Attachment https://bugzilla.mozilla.org/attachment.cgi?id=8369843 in
https://bugzilla.mozilla.org/show_bug.cgi?id=966840 shows the
difference between some style fixes I did by hand for
nsMemoryReporterManager.{cpp,h} and what clang-format produced.

One big issue was that clang-format changes lots of whitespace in the
middle and the end of lines. Some examples follow, where the '-' lines
show what the code looked like after my changes, the '+' lines show
what the code looked like after clang-format's changes, and you'll
want to view this in a fixed-width font...

- nsresult RegisterReporterHelper(nsIMemoryReporter* aReporter,
- bool aForce, bool aStrongRef);
+ nsresult RegisterReporterHelper(nsIMemoryReporter* aReporter, bool aForce,
+ bool aStrongRef);

and

- uint32_t mGeneration;
- nsCOMPtr<nsITimer> mTimer;
- uint32_t mNumChildProcesses;
- uint32_t mNumChildProcessesCompleted;
- nsCOMPtr<nsIHandleReportCallback> mHandleReport;
- nsCOMPtr<nsISupports> mHandleReportData;
+ uint32_t mGeneration;
+ nsCOMPtr<nsITimer> mTimer;
+ uint32_t mNumChildProcesses;
+ uint32_t mNumChildProcessesCompleted;
+ nsCOMPtr<nsIHandleReportCallback> mHandleReport;
+ nsCOMPtr<nsISupports> mHandleReportData;

and

#if defined(MOZ_MEMORY)
-# define HAVE_JEMALLOC_STATS 1
-# include "mozmemory.h"
-#endif // MOZ_MEMORY
+#define HAVE_JEMALLOC_STATS 1
+#include "mozmemory.h"
+#endif // MOZ_MEMORY

In other words, lots of unnecessary churn, and the clang-format
results are IMO inferior. Combine that with the fact that clang-format
as configured only changes whitespace -- no brace insertion, no
parameter/member renaming, etc, etc -- and even using it as a first
step prior to hand-written changes is looking dubious, IMO.

Can we just allow hand-written fixes and move forward, please?

Nick

[Ehsan Akhgari]

On 2/10/2014, 4:32 AM, Nicholas Nethercote wrote:
> Can we just allow hand-written fixes and move forward, please?

Absolutely. I don't see any reason to enforce the usage of a tool as a
criteria for accepting a patch.

If somebody wants to contribute patches to clang-format to make it match
our needs better, that would be great too! But we don't need to block
on that.

Cheers,
Ehsan

[Nicholas Nethercote]

On Wed, Feb 12, 2014 at 7:14 PM, Ehsan Akhgari <ehsan....@gmail.com> wrote:
> On 2/10/2014, 4:32 AM, Nicholas Nethercote wrote:
>>
>> Can we just allow hand-written fixes and move forward, please?
>
> Absolutely. I don't see any reason to enforce the usage of a tool as a
> criteria for accepting a patch.

In this spirit, I've updated my hand-written patches in bug 956199.
Happily, they are XPCOM patches, so asking bsmedberg to review is
doubly appropriate.

Nick