I’ll be upfront and admit that I was the most vocal advocate for including extensive static analysis in our continuous integration (CI) workflow on Github. I envisioned a beautifully and uniformly formatted codebase free from many common C coding errors that would be a great project to show off to future employers. In the end I think it will have been worth it, but there have certainly been some growing pains along the way.
Growing Pain One
Because I was the group member most enthusiastic about static analysis, I volunteered to get static analysis up and running in our codebase. In my previous post I wrote about how we selected our compiler, and I said that we selected clang in part because of its strong support for static analysis tools, including clang-format and clang-tidy. These tools are incredibly useful, and there are numerous resources already available in Github Actions that integrate them into a continuous integration (CI) workflow (we chose cpp-linter-action). These tools proved almost too powerful for our needs, and wading into using them with limited experience using them has been a bit of a challenge.
Here’s the main challenge: both clang-tidy (a static analysis tool) and clang-format (a formatting tool) are highly configurable. There are, of course, default settings in each case that make use of standard options for both static analysis checks (things like default verbosity of errors in clang-tidy etc) and formatting (clang-format defaults to LLVM coding standards). There are also various built-in options to choose from (we decided to use the GNU coding standards built in to clang-format). However, after selecting our baseline configurations we realized that there were a few tweaks we wanted to make related to things like how we handle spaces and parentheses. To make changes of this nature requires wading into verbose documentation to make tiny changes to formatting documents (walls of text, really) like this one:
It’s hard to figure out how to get the changes right, and testing those changes in the CI process requires a loop of pushing changes to the formatting documents and then seeing if they match what you want (wash, rinse, repeat). Not too hard, but tedious and time-consuming nonetheless. Luckily we didn’t have too many modifications to make, and I’m optimistic we have our checks set up the way we want them. That being said, it was a bit more of a headache than I anticipated.
Growing Pain Two
Perhaps the key lesson I have learned so far is to set up static analysis in the CI workflow as early as possible, ideally before writing any code. In hindsight this seems obvious, but in our case we had already written some code before I was able to get static analysis up and running. There were two major consequences of this. First, as soon as static analysis was implemented, our existing codebase was not in compliance with our static analysis checks. This remained the case as I made small changes to our checks based on group feedback, which meant that for a period of a few days, all of our static analysis checks were failing (i.e. code wasn’t being deployed). This wasn’t a huge issue given the nature of our project, but it would be a big problem if we were deploying a web application or something like that. It also would have been a bigger problem if we were working on a larger codebase as that would have increased the amount of time necessary to bring our codebase into compliance.
Second, once our static analysis checks were set up as we wanted them, we needed to go back and reformat code so that it would comply with the new checks. Because I had introduced this new requirement to our codebase, I volunteered to handle this issue. As noted above, this task wasn’t too herculean in part because our codebase wasn’t very large at this point. I was also aided by our tools, especially clang-format. Clang-format can reformat code according to your format specifications. All I needed to do to bring our code into compliance from a format perspective was pull down the latest code, run clang-format, and then push the changes back to Github. The other issues were harder to track down (though cpp-linter-action helpfully adds comments that point to the problem). Resolving these was a combination of reading error messages and editing code as necessary or amending our code to allow certain warnings to be ignored. For example, a readability module in clang-format doesn’t like our use of raw hexadecimal (e.g. 0x1a) because it looks like a magic number where a named variable might be better. This is ubiquitous in our code for good reason, so we decided to ask clang-tidy to ignore those lines. Again, this didn’t take too much time, but it is time that could have been saved if we had put our static analysis checks in place before coding.
Growing Pain Three
Growing pain three really follows from growing pains one and two. Various bumps in the road have come up over the course of the last week or two that have slowed down code integration, and most of these issues relate to the relative complexity of the static analysis checks being performed (growing pain one) and the timing with which those checks went into effect (growing pain two). In the first case, clang-format and clang-tidy are combing through the code looking at a dizzying array of things, which means that our checks can throw errors and fail checks for all sorts of reasons. While it is true that our static analysis checks are looking for things like tabs vs spaces, it simply wasn’t the case that I could (digitally) hand my group a list of bullet points to guarantee that their code would pass all the checks — it is checking far too many things to do that. This can make it difficult to write code that passes static analysis checks the first time, which can slow code integration. However, this issue is getting better over time as our teams learns the rules, and I’ve been able to help my team get up to speed with the tools that make this process easier (even if they still bring me errors that leave me scratching my head).
In the second case, due to the poor timing of our addition of static analysis checks to our CI workflow, for a period of time we weren’t all working from the same static analysis settings. This led to confusion and unexpected errors as people merged their code. This occurred because the formatting instructions are themselves version controlled, which means that different branches were subject to different static analysis checks, so someone could push code to their branch that passed the static analysis checks, then try to merge the code and fail the checks while trying to merge. All of our branches are now performing the same static analysis checks and this issue is resolved (knock on wood), but this issue could have been avoided if everything had been put in place earlier, and it did slow us down.
Conclusion
In the short term, getting static analysis up and running in our CI workflow has taken up more of my time than I expected and would have liked. In the medium term, I think these static analysis checks will save us time, prevent errors, and result in a higher quality codebase. In the long term, I have learned a lot about how to best integrate static analysis tools in a CI workflow, which will benefit both me and future collaborators.
Leave a Reply