On 1/5/2014 8:34 PM, Nicholas Nethercote wrote:
With these ideas in mind, a goal is clear: convert non-Mozilla-style code to
Mozilla-style code, within reason.
One thing that is worth pointing out is that there are three distinct
classes of style rules, which have different implications for conversion
and unification of guidelines:
* Formatting changes, e.g., number of characters in a line, indent
width, brace positioning. By definition, these are invisible to the
compiler (for any language other than whitespace-sensitive languages
like Python or Make), so there are no issues with reformatting
automatically.
* Naming conventions. This is where our multitude of styles is most
evident, but it also creates compatibility issues, particularly for code
that is part of the public API (namely, JS code and XPIDL binary code
generation).
* Other structural rules. Examples here include situations like "don't
use NULL, use nullptr", "always brace an if/else/for/while statement",
etc. These rules are harder to automatically enforce.
There are two notions that block this goal.
- Our rule of thumb is to follow existing style in a file. From the style
guide:
"The following norms should be followed for new code, and for Tower of Babel
code that needs cleanup. For existing code, use the prevailing style in a
file or module, or ask the owner if you are on someone else's turf and it's
not clear what style to use."
This implies that large-scale changes to convert existing code to standard
style are discouraged. (I'd be interested to hear if people think this
implication is incorrect, though in my experience it is not.)
I propose that we officially remove this implicit discouragement, and even
encourage changes that convert non-Mozilla-style code to Mozilla-style (with
some exceptions; see below). When modifying badly-styled code, following
existing style is still probably best.
However, large-scale style fixes have the following downsides.
- They complicate |hg blame|, but plenty of existing refactorings (e.g.
removing old types) have done likewise, and these are bearable if they
aren't too common. Therefore, style conversions should do entire files in
a single patch, where possible, and such patches should not make any
non-style changes. (However, to ease reviewing, it might be worth
putting fixes to separate style problems in separate patches. E.g. all
indentation fixes could be in one patch, separate from other changes.
These would be combined before landing. See bug 956199 for an example.)
Whitespace-only changes are easily ignorable in blame tooling, and I
thus consider them a non-issue, particularly since the CVS/hg blame
divide still hurts me when doing historical blame research.
- There is an semi-official policy that the owner of a module can dictate its
style. Examples: SpiderMonkey, Storage, MFBT.
There appears to be no good reason for this and I propose we remove it.
Possibly with the exception of SpiderMonkey (and XPConnect?), due to it being
an old and large module with its own well-established style.
A related issue that is worth bringing up is that we have wildly
different style guides for different languages. The effective style for
Mozilla code I've worked with has been "brace on newline for C/C++ code
but at end of line for JS code" (style has changed somewhat I think in
general, but the "use prevailing style" rule has really confounded the
issue).
Finally, this is a proposal only to reduce the number of styles in our
codebase. There are other ideas floating around, such as using automated tools
to enforce consistency, but I consider them orthogonal to or
follow-ups/refinements of this proposal -- nothing can happen unless we agree
on a direction (fewer styles!) and a way to move in that direction (non-trivial
style changes are ok!)
Style guidelines are only effective if they can be enforced. Without
education of reviewers and probably automated enforcement tools, any new
style regime will only last a short while before style proliferation
happens again.
--
Joshua Cranmer
Thunderbird and DXR developer
Source code archæologist
_______________________________________________
dev-platform mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-platform
