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

Post by 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
"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

[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

Post by 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

Post by 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
"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

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

[Robert O'Callahan]

Post by Bobby Holley
* 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

[smaug]

Post by Bobby Holley
I think trying to change SpiderMonkey style to Gecko isn't a great use of
* 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.

Post by Bobby Holley
* 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

Post by 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

Post by 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
"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

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

[Gavin Sharp]

A concise summary of the changes you're proposing would be useful -
here's my attempt at one.
* 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

On Sun, Jan 5, 2014 at 9:34 PM, Nicholas Nethercote

Post by 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
"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
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

[Adam Roach]

Post by Gavin Sharp
A concise summary of the changes you're proposing would be useful -
here's my attempt at one.
* 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

Post by Adam Roach

Post by Gavin Sharp
A concise summary of the changes you're proposing would be useful -
here's my attempt at one.
* 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

* Don't reformat third-party code.
/a

[Joshua Cranmer 🐧]

Post by Nicholas Nethercote
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.

Post by Nicholas Nethercote
There are two notions that block this goal.
- Our rule of thumb is to follow existing style in a file. From the style
"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.

Post by Nicholas Nethercote
- 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).

Post by Nicholas Nethercote
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.

[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

Post by 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
"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
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

[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

Post by 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).
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

Post by 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
"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
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

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

[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

Post by 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).
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

Post by 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
"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
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

[Ehsan Akhgari]

Post by Axel Hecht
Hi,
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.)

Post by Axel Hecht
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

Post by Axel Hecht

Post by 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).
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
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

Post by 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
"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
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

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

[Adam Roach]

Post by Axel Hecht
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?

[Chris Peterson]

Post by Martin Thomson
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]

Post by Nicholas Nethercote
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.

[Martin Thomson]

Post by Karl Tomlinson
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.

Post by Karl Tomlinson
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.

Post by Karl Tomlinson
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.

Post by Karl Tomlinson
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.

[Karl Tomlinson]

Post by Martin Thomson
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.

Post by Martin Thomson
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.

Post by Martin Thomson
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]

Post by Karl Tomlinson

Post by Martin Thomson
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.

[Gregory Szorc]

Post by Daniel Holbert

Post by Karl Tomlinson

Post by Martin Thomson
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.
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.

[Karl Tomlinson]

Post by Gregory Szorc
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]

Post by Karl Tomlinson

Post by Gregory Szorc
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.

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]

Post by Karl Tomlinson
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

[Mike Hoye]

Post by Karl Tomlinson
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]

Post by Karl Tomlinson

Post by Gregory Szorc
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.

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

Cheers,
Ehsan

[Martin Thomson]

Post by Karl Tomlinson
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]

Post by Martin Thomson
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

[Brian Smith]

On Sun, Jan 5, 2014 at 6:34 PM, Nicholas Nethercote

Post by Nicholas Nethercote
- 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.

Post by Nicholas Nethercote
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

[Martin Thomson]

Post by Brian Smith
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]

Post by Brian Smith
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.

Post by Brian Smith
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

Post by Brian Smith
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.

Rob

[Jason Orendorff]

Post by Brian Smith
On Sun, Jan 5, 2014 at 6:34 PM, Nicholas Nethercote

Post by Nicholas Nethercote
- 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

[Joshua Cranmer 🐧]

Post by Robert O'Callahan
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]

Post by Joshua Cranmer 🐧

Post by Robert O'Callahan
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.

Post by Joshua Cranmer 🐧
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]

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.)

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?

[Joshua Cranmer 🐧]

Post by smaug

Post by Joshua Cranmer 🐧

Post by Robert O'Callahan
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.1385151580.23840.dev-***@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.

[Ms2ger]

Post by Joshua Cranmer 🐧

Post by smaug

Post by Joshua Cranmer 🐧

Post by Robert O'Callahan
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.)

(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.

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]

Post by Ms2ger

Post by Joshua Cranmer 🐧
(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.

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.

I'm happy about getting rid of NS_ENSURE_*.

-David

[Benoit Jacob]

Post by L. David Baron

Post by Ms2ger

Post by Joshua Cranmer 🐧
<news://
(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.

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.

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

Post by L. David Baron
-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)
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

[Neil]

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...

[Benoit Jacob]

Post by Benoit Jacob
I would like a random voice in favor of deprecating NS_ENSURE_* for the

Post by Benoit Jacob
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...

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

Post by Benoit Jacob
--
Warning: May contain traces of nuts.
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

[Kyle Huey]

Post by Benoit Jacob
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]

Post by Kyle Huey

Post by Benoit Jacob
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.

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]

Post by Kyle Huey
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]

Post by Kyle Huey

Post by Benoit Jacob
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

Post by Kyle Huey
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]

Post by Kyle Huey
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

[Neil]

Post by Benoit Jacob
I'm scanning a function for possible early returns

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

[Joshua Cranmer 🐧]

Post by Benoit Jacob
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).

[Gavin Sharp]

I support getting rid of NS_ENSURE_*.

Gavin

Post by Ms2ger

Post by Joshua Cranmer 🐧

Post by smaug

Post by Joshua Cranmer 🐧

Post by Robert O'Callahan
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.)

(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.

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
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

[Jeff Walden]

Post by Robert O'Callahan
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

[Jeff Walden]

Post by Jason Orendorff
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

[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

Post by 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
"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
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

[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.

Post by 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

Post by 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
"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
_______________________________________________
dev-platform mailing list
https://lists.mozilla.org/listinfo/dev-platform

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

[Henri Sivonen]

Post by 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.

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.

Post by Patrick McManus
* 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.)

[Benjamin Smedberg]

Post by Nicholas Nethercote
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.

Post by Nicholas Nethercote
- 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.

Post by Nicholas Nethercote
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

[Nicholas Nethercote]

On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg

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

[Mike Hommey]

Post by Nicholas Nethercote
On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg

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.

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

Mike

[Nicholas Nethercote]

Post by Nicholas Nethercote
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

[Nicholas Nethercote]

On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg

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

[Benjamin Smedberg]

Post by Nicholas Nethercote
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

[Anthony Jones]

Post by Nicholas Nethercote
On Tue, Jan 7, 2014 at 12:31 PM, Benjamin Smedberg

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.

Post by Nicholas Nethercote

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.

Post by Nicholas Nethercote
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.

Post by Nicholas Nethercote
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]

Post by Anthony Jones
In my opinion clang-format is good enough.

That's awesome!

Post by Anthony Jones
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.

Post by Anthony Jones
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?

Post by Anthony Jones
bool
MyClass::MyFunction()
vs
bool MyClass::MyFunction()

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

[Neil]

Post by Bobby Holley

Post by Anthony Jones
bool
MyClass::MyFunction()
vs
bool MyClass::MyFunction()

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.

[Karl Tomlinson]

Post by Anthony Jones
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.

Post by Bobby Holley
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.

[Gregory Szorc]

Post by Anthony Jones

Post by Nicholas Nethercote

Post by Benjamin Smedberg
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

[Anthony Jones]

Post by Gregory Szorc
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.

Post by Gregory Szorc
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]

Post by Anthony Jones
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

[Anthony Jones]

Post by Anthony Jones
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]

Post by Anthony Jones

Post by Bobby Holley
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]

Post by Nicholas Nethercote
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.

Post by Nicholas Nethercote
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

[Nicholas Nethercote]

On Fri, Jan 31, 2014 at 6:08 AM, Benjamin Smedberg

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]

Post by Nicholas Nethercote
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]

Post by Ehsan Akhgari

Post by Nicholas Nethercote
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

[Anthony Jones]

Post by Nicholas Nethercote
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]

Post by Anthony Jones

Post by Nicholas Nethercote
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.

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

[Bobby Holley]

Post by Anthony Jones

Post by Nicholas Nethercote
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.

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]

Post by Bobby Holley
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

[Kyle Huey]

Post by Anthony Jones

Post by Nicholas Nethercote

Post by Benjamin Smedberg
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
https://lists.mozilla.org/listinfo/dev-platform

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

- Kyle

[Sylvestre Ledru]

Post by Kyle Huey

Post by Anthony Jones

Post by Nicholas Nethercote

Post by Benjamin Smedberg
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

[Zack Weinberg]

Post by Anthony Jones
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

[Anthony Jones]

Post by Kyle Huey
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]

Post by Anthony Jones

Post by Kyle Huey
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
C:\> cl /E MyFile.cpp
Pretty much does the same thing as `gcc -E` or `clang -E`.

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

Anthony

[Chris Peterson]

Post by Benjamin Smedberg
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]

Post by Chris Peterson

Post by Benjamin Smedberg
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/

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

Cheers,
Ehsan