(In case the formatting doesn't survive transit, a copy of this note can be
found here:


To improve developer productivity, we are planning to a) start
automatically formatting all Gecko C++ code with clang-format, and b)
switch to the Google C++ style
<https://google.github.io/styleguide/cppguide.html> and retire the Mozilla
C/C++ coding style
These changes are being tested and verified with a small part of the
codebase before rolling out to the entire tree.


Mozilla has been moving towards using more automated tools for checking and
rewriting code. For example, eslint <https://eslint.org/> (JS), checkstyle
<https://github.com/checkstyle/checkstyle> (Java), flake8
<http://flake8.pycqa.org/en/latest/> (Python) are now Tier-1 in the CI. We
also deployed C/C++ static analysis as part of the development process.

In parallel, tooling to reformat C & C++ is now used in a wide variety of
projects like Chromium, LLVM and MongoDB. Mozilla has historically had a
coding style but it was never really enforced through tooling. In addition,
the Mozilla style is incomplete and used only in our projects; by contrast
the Google style is very complete, well supported in automated tooling, and
widely used by projects inside and outside of Google such as Chromium,
Tensorflow, and MySQL.

Because of the lack of checks and automation, we now have significant
inconsistencies in the style of our codebase.  On top of that, we have been
spending a lot of time talking about and adjusting whitespace. This wastes
human time when writing, reviewing, and discussing code.

Last but not least, being able to automatically format whitespace in our
C/C++ code is helpful for other types of automated rewrites so we can fix
wide-scale problems in our codebase using static analysis tools.  Right now
such fixes are difficult or impossible because of inconsistencies in the
Mozilla C++ coding style and lack of support in clang-format and other
tools. Addressing this issue is the first step in unblocking those future


During patch authoring, code reviews, and mailing list discussions, Mozilla
developers have traditionally spent a lot of time on minor issues that
could easily be fixed through automated processes. Our Engineering
Operations and Product Integrity groups have helped to improve developer
productivity in this area through tools such as static analysis
<https://static-analysis.moz.tools/#/stats>, linting via Phabricator
reviewbot <https://phabricator.services.mozilla.com/p/reviewbot/>, and codebase
using clang-format. We will build on this foundation by applying coding
style checks and automated rewrites throughout the authoring process
(pre-commit, commit, review, landing).


We’re announcing the following changes to our coding style and its
enforcement through our commit policy:


   We will check conformance with coding style rules upon commit, review,
   and landing so that issues can be easily addressed or fixed through
   automation. The preference will be to enforce style issues as early as
   possible (before landing) to avoid late surprises.


   We will migrate to the Google C++ coding style to encourage more
   consistent code and re-use a wider array of existing tools.  As part of
   this migration, we will retire the existing Mozilla C/C++ Coding Style. The
   intention behind retiring the current Mozilla style is to just stop
   maintaining it as an active coding style going forward, but the
   documentation for this coding style will be kept intact.


   We will automatically enforce restrictions on formatting of whitespace
   (such as indentation and braces). For stylistic features other than that
   (such as naming of functions and #include order), Google C++ style will be
   permitted but not initially enforced, and consistency with surrounding code
   should take precedence.

These changes have been approved both by Firefox senior engineering
leadership and the Firefox Technical Leadership Module
<https://wiki.mozilla.org/Modules/Firefox_Technical_Leadership>.  This work
is being tracked in bug 1188202


On Monday this week, we pushed a patch
<https://bugzilla.mozilla.org/show_bug.cgi?id=1204606> to an integration
branch to rewrite a section of the tree (dom/media) in the Google C++ style
using clang-format.  This first step is intended to be a smoke test step,
designed to test the process of doing the rewrite and test the waters in a
small controlled environment with a small team of developers who have
volunteered to help us test this change. We haven’t found any major
surprises yet.

Assuming that everything goes well, on Friday, November 30, we will push a
patch to mozilla-central and the integration branches syncing into it in
order to rewrite the entire tree using clang-format with the Google C++
Coding Style.

We want to do this work right before a merge (when nightly version moves to
beta) to limit the impact on the beta uplift process. Conveniently, this
happens to be the Friday just before the Mozilla All Hands when a lot of
Mozilla developers will be traveling, so we are hoping that would be a less
busy time for landing this change.

To mitigate the impact on the developers who backport fixes to ESR, on
Friday, December 14, we will also reformat the ESR60 code base.

Reformatting Code Locally

To reformat code locally

On a directory or a file specified by <path>:

$ ./mach clang-format -p <path>

On a modified tree (where hg diff or git diff) return a non-empty output:

$ ./mach clang-format

See the documentation for more information
.  There are integrations available for most popular code editors allowing
developers to easily format the code they’re working on at the time they’re
making changes to it.  The documentation links to several integrations for
the popular editors.

Rationale Behind Current Plan

We understand that switching coding styles can be a contentious choice, and
not one that everyone may be happy about initially.  Many people would
agree that having a consistent style across the code base is valuable, but
that’s not where we are now. To a great degree, that has been caused by
lack of good tools to enforce our existing coding style.  By contrast,
there is tooling to enforce quite a bit of the Google C++ Style, and this
is why we would like to convert entirely to this coding style -- perhaps
with some exceptions for Mozilla-specific constraints. That is something
that we are going to look at seriously going forward.

We have decided here to start this conversion by converting the whitespace
related portions of our coding style mechanically using clang-format as a
first step. Of course, other strategies for this conversion are also
possible, such as waiting until such a time when we have the tooling ready
for switching wholesale to the Google C++ Style.  But we have decided that
by allowing the usage of the Google C++ Style in new areas, we are taking
an incremental step towards that ultimate goal by allowing (not requiring)
people to write code immediately after our switch that is consistent with
our newly adopted coding style and can be verified as such using tools. The
intention here is not that people intermix Google C++ Style and Mozilla
Style code on a per function or even a per file basis but rather that they
be free to do so when starting new, self-contained work, or to convert an
entire module.

Providing Feedback

This post is intended as an announcement, but we do welcome your feedback
on this, both at a high level and at the technical level.  For high level
feedback please reach out to t...@mozilla.com and CC myself.  For technical
feedback (e.g. bugs about the conversion process) please file bugs under bug
clang-format <https://bugzilla.mozilla.org/show_bug.cgi?id=1188202>.

Last but not least, coding style is a living organism. This change doesn’t
mean that the coding style will remain forever this way; to the contrary
this will give us the opportunity to easily change our coding style and
apply the change on the code base.

I’d like to extend my gratitude to many who have helped getting us to this
point, including Sylvestre Ledru, Peter Saint-Andre, Andi-Bogdan Postelnicu
and Birunthan Mohanathas.

Best regards,



Can I see what it looks like?

With an up-to-date repository, you can use

$ ./mach clang-format -p <path>

A formatted repository is available on github:
https://github.com/sylvestre/gecko-dev/ to visualize the changes.

Is it going to break something?

Of course, every software has bugs but all the testing that we have done
didn’t show any sign of regression.

You will break the blame on VCS with this change

Yes and no. Of course, just like many tree-wide mass changes in the past
(e.g. the MPL2 header update), this will remain in the log.

Mercurial and Git both support a -w argument to ignore whitespace with

In addition, modern versions of Mercurial have `hg annotate --skip
<revset>` which allows you to specify a revset used to select revisions to
skip over when annotating.

Last but not least, we will tag the changeset’s commit message with
“skip-blame” so that Mercurial would automatically ignore the reformat
changeset for blame operations.

Why doing that in a single patch instead of doing that file per file or
module per module?

We are doing that into a single patch for several reasons:


   We will be done quickly

   We will have a single revision to ignore in the annotate/blame

   We can tag this magic revision to run some tasks on it (local reformat
   to avoid conflicts)

clang-format doesn’t add missing braces, is that normal?

clang-format only does whitespace changes. It doesn’t add or remove code.

However, this can be done through ./mach static-analysis. For example:


   ./mach static-analysis check --check
   google-readability-braces-around-statements --fix js/src/wasm/

This checker is enabled at review phase
in Phabricator.  Note that the --fix argument here means that the
static-analysis job will automatically rewrite the local tree in order to
add braces around statements anywhere it finds those issues, no need to do
it manually!

Does everything has to be formatted?

Yes, all of the C/C++ code that we maintain in mozilla-central has to be

There are cases, like some structure declarations, for which we want to
ignore the formatting. Example:


In that case, add:

// clang-format off

code not to be formatted

// clang-format on

My component has some very specific needs with very good reasons, can I
customize the coding style?

No, going forward we are planning to unify all of our code (excluding
third-party code and any potential code owned by us that is developed
outside of mozilla-central) under the same coding style.  If there are
individual blocks within your code that require custom formatting for good
reasons, please see the previous question for how to turn clang-format off
for specific sections.

If you see issues about the style change that make you unhappy and you
think affect the code in the areas you work on in negative ways, please
consider spending some time to try to get used to the change.  But if after
some time the change still seems inferior compared to before please raise
the issues and we will work to figure out how to address those problems to
the best of our ability.

Can I ignore just a file?

If you have a good reason, yes. For example, in some cases, we have some
tooling which parses the C/C++ code with a very specific format. For this,
just add it into .clang-format-ignore
<https://dxr.mozilla.org/mozilla-central/source/.clang-format-ignore> file
with a comment explaining why it should be ignored.

Please note that examples of what constitutes a good reason for excluding
single files would be external tools needing to parse the file in specific
ways, or cases where clang-format can’t deal with the input (such as bug
1342657 <https://bugzilla.mozilla.org/show_bug.cgi?id=1342657>).  Examples
of what does not constitute a good reason would be personal preferences.

What about variable naming convention?  What about function/class/etc.
naming conventions?

With this change, we are not aiming to enforce any naming conventions at
this time.

What about third party code?

Third party code will not be touched. See this list:

This is mirrored here for clang-format to ignore these directories:


If you are aware of third-party code developed outside of mozilla-central
that isn’t listed here, please file a bug as soon as possible to get it
added to this list.

Why are we switching to the Google C++ coding style?

The current Mozilla C++ style
is not well supported by tools (in particular clang-format) and it has
been difficult
to <https://reviews.llvm.org/D37979> upstream patches
<https://reviews.llvm.org/D12921#247649> to clang-format to handle the
Mozilla style. By contrast, the Google C++ style used in a number of
projects (parts of NSS
<https://hg.mozilla.org/projects/nss/file/tip/cpputil/.clang-format>, MySQL
and many others
<https://github.com/search?q=BasedOnStyle%3A+Google&type=Code>) and well
supported in clang-format, linters
and clang-tidy checks
<https://clang.llvm.org/extra/clang-tidy/checks/list.html>. Defining and
maintaining our own C++ coding style probably isn’t the best use of our
time, and switching to the Google style will help us improve the
productivity of our developers, make onboarding easier, and pave the way
for more automation of trivial but time-consuming code fixes.

The feature foo from the coding style is missing in clang-format, what
should I do?

Going forward, our precise coding style will be the result of formatting
our tree with clang-format, so by definition our code, when formatted with
clang-format, will be following our new coding style.

In cases where you have found bugs in how clang-format implements the
Google C++ Style, please report those issues to the LLVM project:
https://bugs.llvm.org/.  When the fixes are available in new versions of
clang-format, we’ll pick up the resulting reformatting automatically
through version upgrades.

In case you have feedback on the Google C++ Style Guide itself, please
consider filing an issue/PR in the upstream repository

How will we be sure we don’t regress?

For a while we’ve been running clang-format linting checks at review time
on Phabricator in shadow mode
<https://static-analysis.moz.tools/#/check/clang-format.lint> (executed but
not published). Over the next few weeks, we will enable verifications at
review phase on on Phabricator.

In parallel of that, we will offer hooks that can be enabled locally to
check for formatting.  We’re hoping to enable a hook on hg.mozilla.org to
automatically reject improperly formatted commits soon after that.  This
will prevent badly formatted code from being pushed to hg.mozilla.org in
the first place.

What will happen to my local changes? Will I have to rebase everything?

In order to mitigate the impact for pending changes, we developed a
mercurial extension called hg format-source
<https://pypi.python.org/pypi/hg-formatsource> to locally reformat the
local changes before the big patch is pulled. This will fix most of the
conflict issues.

We will share more details later.

For git users, we are working on providing a similar script based on the
one developed by the Mongo project

Playing with clang-format, I noticed changes in the output between
versions, how will we manage that?

Because clang-format evolves, this is true that the output can change from
a version to another. This is why ./mach clang-format downloads an specific
artifact from the taskcluster CI. At this date, we are using clang-format
version 7.

As the Phabricator review bot is using ./mach clang-format to run the
analysis, the same version is used.

The version of the LLVM tools used for this purpose is defined in-tree for
and Windows

What will happen to the downstream consumers of the ESR repository after
ESR60 is rewritten with clang-format?

We are aware of the impact of this plan on Firefox derivatives but we think
it is worth it the pain.  The goal behind rewriting the ESR branch is to
make it easier to uplift patches from trunk onto the ESR branch, and this
change should make things easier for downstream consumers creating Firefox
derivatives based on the ESR60 codebase.

Such consumers still have the capability to run the following pseudo code
in the derivative project to generate up to date patches if they have local
modifications on top of ESR.  In the example below, fx-esr60-reformatted is
the SHA1 hash of the Mercurial changeset that has rewritten the ESR60
repository with clang-format.  This pseudo code shows the necessary steps
if the local modifications are applied on top of fx-esr60-reformatted.


   # Rewrite the locally modified tree
   ./mach clang-format -p <path of the file touched>

   # Generate a diff of the local changes against
   hg diff -r [fx-esr60-reformatted]
dev-platform mailing list

Reply via email to