Succeeding With ClangFormat, Part 2: The Big Reformat

When properly integrated into a toolchain, ClangFormat can entirely do away with time wasted on discussion and enforcement of code formatting. In part 1 of this series, I laid out the case for doing so, the factors that doomed our prior attempt, and the approach we took to get it right the next time. In this part I’ll walk through all the details that have to be considered before drawing up a functional specification and reaching the next milestone: codebase conversion.

Setting the format

Landing on a format was surprisingly easy, considering how contentious formatting choices can be. In this area, MongoDB has the benefit of being towards the larger end of team size. In a large shop, developers seem more understanding that "there is a way of doing things" that might not be their personal preference. But regardless of your team's size, everyone has to agree on the fundamental principle that a standard is more important that which standard. Start your initiative with obtaining buy-in, and demonstrate your commitment to solving disputes fairly, and you will find this step is not as fraught as you might expect.

Assembling the bikeshed squad

In most organizations, and certainly at MongoDB, the whole team cannot be involved in this decision, it is simply a waste of time and can be an emotional drain to have everyone debate the format rules. Given the almost religious fervor people have for this topic, having a large discussion group may lead to a standoff between zealots over a particularly divisive formatting topic. These standoffs can be avoided by setting ground rules for the decision-making process and conflict resolution beforehand. This can be simple as saying any disputes will be put to a vote. But whatever the rules, people must understand them ahead of time. Your format-setting squad should be composed entirely of people who are either a) implementing the formatting tool (which was my role in our ClangFormat project), or b) managing developers. We decided that some of our managers would be used as the decision committee (a total of 6 people), and we made sure they could all fit in a room if an in-person discussion was needed.

Setting the tip of the spear

There are a lot of presets and knobs to play with in ClangFormat, and it should fall to a single individual person to take a first pass. In our project, that was my job, so I put together a file of sample code that demonstrated various options, and sent it for code review to the bikeshed team. This allowed them to comment about aspects of the code layout they did not like, and I could then adjust the rules accordingly. For us, we were left with only two options that no clear agreement were reached on (AllowShortFunctionsOnASingleLine, and BreakConstructorInitializersBeforeComma). For these final options, we simply put it to a formatting team-wide vote by email (and decided on Empty, and false respectively).

Coping with your tool's limitations

While ClangFormat has a rich set of configuration options, you may find you cannot configure it to match your existing code format. Our previous style was one such case, a hybrid of what ClangFormat calls the Google and Stroustrup styles. In particular, we had two unusual rules for formatting code that ClangFormat did not support.

First, inside namespaces, we required a fixed 4-space indentation for code regardless of the number of levels of namespace nesting:

namespace mongo {
namespace repl {
    void main() {
    }
} // namespace repl
} // namespace mongo

Second, we required the opening curly brace to be attached to the function definition, but the else to always start on a new line separate from the closing curly bracket:

if (foo()) {
}
else {
}

Then there are the special cases where your team's unique specializations conflict with your tools assumptions about what your tokens mean. Yeah, I'm talking about macros. When are macros ever hassle-free? In our case, we have a macro named BSON that is used to build BSON documents, which uses the operator <<.(Think data-preserve-html-node="true" std::strstream paired with str() in a single macro). The << data-preserve-html-node="true" operator will be consumed by the macro pre-processor, and because we use it to delineate JSON field and value pairs, we want each line to read << "field" << "value". But ClangFormat expects the << data-preserve-html-node="true" operator to be used in an iostreams context, and so tends to join lines such that there is only one << data-preserve-html-node="true" operator per line... which does not suit our intentions.

A good example of this is found in this bug report:

+++
@@ -921,10 +921,14 @@
 /** A complex _id expression. */
 class ComplexId : public CheckResultsBase {
     std::deque<Document> inputData() {
-        return {DOC("a" << "de"
-                 << "b" << "ad"
-                 << "c" << "beef"
-                 << "d" << ""),
+        return {DOC("a"
+                    << "de"
+                    << "b"
+                    << "ad"
+                    << "c"
+                    << "beef"
+                    << "d"
+                    << ""),
                 DOC("a"
                     << "d"
                     << "b"

We experimented with customizing ClangFormat to match our existing rules. The ClangFormat code was easy to work with, but ultimately, the ongoing cost of maintaining a fork is not worth the effort to match our old format. Instead, we adapted our existing rules to match what ClangFormat was already capable of. We strongly recommend that you do the same.

There are several other examples of this mismatch between automated tooling and developer expectation; see SERVER-19670 and SERVER-19184. If a developer is truly unhappy with the final formatting done by ClangFormat, the tool provides a way to disable formatting, but in practice, we find developers would rather let ClangFormat format the code, even when they don’t love the results. As of today, there are only 11 cases in our code base (here’s one example) where we stepped in and disabled formatting for small blocks of code. Given there are thousands of files in our codebase, I’d call that a strong endorsement of ClangFormat’s capabilities.

ClangFormat pragmatics

ClangFormat ships with several defaults for format including ones based on the Google C++ Style Guide, the Linux kernel, and Stroustrup’s style. (BSD’s Kernel Normal Form is not available in 3.6, but will be available in 3.8.) Using a base style is far easier than deciding on each option one by one, but if you have an existing style to match, as we did, you’ll need to start tuning. For that, .clang-format and ClangFormat configurator are your friends. These tools allow you to tune options one by one while observing how they affect your code.

The wrapper script

In any non-trivial use of clang-format, you will want a wrapper around it. Our script, clang_format.py:

1. Fetches clang-format binaries, from LLVM when it can, and from a MongoDB cache when it cannot

2. Validates clang-format is at the right version when binaries are already present

3. Skips files that we shouldn’t bother formatting, i.e. 3rd party library code, like boost

4. Offers modes for both validating and updating

5. Manages parallel invocations of clang-format to increase throughput

Existing code

The next big decision is what to do about the existing code: Do you leave it as is? If you reformat it, do you do it all in one fell swoop?

Format-only changes

The majority of developers I know believe that the best practice, in day-to-day development, is not to make format-only changes to existing code. They clutter the repository log, and can be disruptive to the use of tools like “git blame” and “hg annotate”, which cannot reliably differentiate between changes to a file that only involve formatting changes versus substantive changes to the code. While these tools can be told to ignore some whitespace changes, they are confused by other changes (like joining or splitting lines).

The downside to this policy is it inevitably creates a code base with different styles across and even within files, as the code formatting style for a code base evolves, and as multiple people work in a code base. And if a tool is rolled out, then to prevent every subsequent checkin from containing a ton of format-only changes, there must be some rule or rules for which files the tool should be applied to and which ones to exclude.

This partial application just creates more work without delivering the full benefit, and we decided to just do one big reformat of the code base. While useful code history is an important issue, a big reformat is not very disruptive to history if it occurs infrequently (e.g. once a year or less), and is applied to the entire code base at that time. If you do it that way, users can easy navigate over these reformat changes in their history navigating tool.

The func spec

The end product of all of this work should be a document every bit as polished and binding as any artifact your team produces for the development of features. For us, this is a functional spec. In it, we detailed the goals and non-goals of the project, the changes to the official formatting guidelines that resulted from the planning phases, and our implementation plan for the big reformat. It also included everything a developer would need to obtain the tools, and instructions for following the new development workflow using them.

Flag day

Once you decide to reformat all the code (and we strongly recommend this), the next step is to plan for the actual big commit of the reformat. We call this “flag day” -- the day you plant a flag in the ground. It is best to do this on a weekend if you can, when freezing the entire codebase will be the least disruptive. Tell your entire team about this weeks in advance, and keep reminding them about it! (We planned ours for the weekend just after the beginning of a sprint iteration.)

Of course, flag day isn’t the first time you reformat the entire codebase, it’s just the first time you commit the results back. During the exploration phase of this project, you will doubtless reformat your codebase many times over as you fine tune, and as you converge on a func spec, you should also be validating that the results are passing tests. By the time flag day arrives, you should have already done a full reformat and completely validated the results for shipping code, at least once.

All the preparation proved itself worth the effort for us, and our flag day reformat went smoothly and without incident.. With that said, there were things we could have done better. And as I said in part 1, the difference between success and failure in this endeavor isn’t doing it once, but building it into your workflow so that rot does not set in.. I’ll talk more about those in the concluding part of this series, so stay tuned.