(In case the formatting doesn't survive transit, a copy of this note can be found here: https://docs.google.com/document/d/1CTaWucldHxEri5BUB1kL4faF4prwPlBa31yHFd-l8uc .)
TL;DR 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 <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style>. These changes are being tested and verified with a small part of the codebase before rolling out to the entire tree. Background 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 improvements. Goals 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 reformats <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style/Formatting_C++_Code_With_clang-format> 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). Details We’re announcing the following changes to our coding style and its enforcement through our commit policy: 1. 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. 2. 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. 1. 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 <https://bugzilla.mozilla.org/show_bug.cgi?id=1188202>. When? 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 <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style/Formatting_C++_Code_With_clang-format> . 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, Ehsan FAQ: 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 annotate/blame. 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 <https://static-analysis.moz.tools/#/check/clang-tidy.readability-braces-around-statements> 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 formatted. There are cases, like some structure declarations, for which we want to ignore the formatting. Example: https://hg.mozilla.org/mozilla-central/rev/7807742373e1 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: https://dxr.mozilla.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt This is mirrored here for clang-format to ignore these directories: https://dxr.mozilla.org/mozilla-central/source/.clang-format-ignore 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 <https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_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 <https://dev.mysql.com/doc/dev/mysql-server/latest/PAGE_CODING_GUIDELINES.html>, Android <https://github.com/search?q=org%3Aaosp-mirror+BasedOnStyle&type=Code>, Tensorflow <https://github.com/tensorflow/tensorflow/blob/master/tensorflow/.clang-format>, and many others <https://github.com/search?q=BasedOnStyle%3A+Google&type=Code>) and well supported in clang-format, linters <https://raw.githubusercontent.com/google/styleguide/gh-pages/cpplint/cpplint.py>, 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 <https://github.com/google/styleguide>. 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 <https://github.com/mongodb/mongo/blob/8b66399b8536dc3120be5444b81ba51d4b8a95e4/buildscripts/clang_format.py> . 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 Linux <https://searchfox.org/mozilla-central/source/build/build-clang/clang-tidy-linux64.json>, Mac <https://searchfox.org/mozilla-central/source/build/build-clang/clang-tidy-macosx64.json> and Windows <https://searchfox.org/mozilla-central/source/build/build-clang/clang-tidy-win64.json> . 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 dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
