Sometimes a code change looks deceptively small. Fixing bugs can be this way. I’ve spent a more than one day staring at a couple of lines of code, trying to figure out a tiny little change that will fix everything.
Other times, a tiny little change will have all the results in the world, especially if it makes someone’s life better. I feel that way about typos in documentation or code comments, for example. For some reason, my brain looks at the words and it’s difficult to focus on the meaning until I can stop focusing on an easy-to-fix typo.
PR 2503 is one of those times when looking at the code change belies the effort that went into it, or at least the effort people thought would go into it.
If you’re reading this and wondering how to get involved with an open source project, building it and looking at any warnings or errors can be really effective. It takes some perseverance and detective skills, but it’s almost always very welcome work, often teaches you something, and only rarely requires days and days of work to see any progress.
These submissions are usually very easy to review too, so as a bonus they might get accepted faster than most other contributions.
The story begins when CosmoRied reported issue 1912 and then opened PR 2484. When compiling the Dogecoin Core, people noticed that the process generated a lot of warnings. This is usually something worth investigating. A warning doesn’t necessarily mean an error, but it means the computer found something a little bit unexpected somehow.
In this case, the computer looked at a group of code called a switch statement and found that it used implicit fallthrough. That link does a pretty good job of explaining what a switch statement does and what fallthrough is, so I won’t repeat it here, except to say two things.
First, the implicit fallthrough behavior of the switch statement as specified in C++ can be surprising; it doesn’t necessarily do what you think. That’s why there’s a warning, to get people to ask if the behavior they asked for is the behavior they wanted. (There’s probably an argument for programming language designers here, but C++ made its choice here over 40 years ago and inherited that choice from C over 50 years ago, so it’s up to us to live with the world we find ourselves in.)
Second, this warning comes from code that Dogecoin doesn’t own. It’s in a file
called tinyformat.h which comes from a project called
tinyformat. That file lives and changes
outside of Dogecoin, and that file gets copied into Dogecoin once in a while.
The feature it provides gets used everywhere, and because the implementation of
that feature is in a header file, the implicit fallthrough warning appears
every time any other file in the Dogecoin core uses the feature.
That’s a lot of warnings!
This is starting to get a little more complicated. I wanted to remove all of the warnings about implicit fallthrough behavior in code, but because the biggest source of warnings came from tinyformat, I knew that was the biggest target.
As an aside, I like this strategy for removing compiler warnings: get rid of the biggest source first, then iterate. That way, everything that’s left is more interesting because it’s more exceptional. When you finally reach the point where there are no warnings left, any new warnings that appear will be obvious because they’re novel.
A comment on issue 1912 suggested using a macro to suppress this warning, because different compilers supporting different versions of the C++ language standard had different approaches. I worked with this for a while, and then I asked myself if this was truly the simplest approach.
You can’t see this in my PR because I never published this idea, but I thought I could define a compiler-specific macro to suppress warnings. That’s not too difficult to do, but it’s a lot more work than I ended up doing. (I think I went looking for a way to do this in Boost, but I don’t remember now.)
I eventually looked at the upstream source of tinyformat itself and saw that
the code had already been updated to use a special comment of // Falls through,
four years earlier.
Updating the file in the Dogecoin core to a newer version of the library was simple and reduced the number of warnings dramatically.
In the process of doing so, Patrick and I realized that the clang compiler supports the same comment mechanism to suppress the warning.
This meant I could use the same style of comment in the other places in the Dogecoin core that generated that warning.
Because no places in the Dogecoin core (at least now that we’ve merged this code) generate implicit fallthrough warnings, we know that any new occurrences of this warning are worth looking at separately. At least as important, we all learned something about this comment and how well it works across compilers.
That all adds up to a satisfying experience: I didn’t have to write much code to get a lot of benefit, the benefit pays off immensely over time, and I learned something about multiple systems that I didn’t know.
