Contents
Today’s post is about an incident with our compiler and a small little feature that sometimes seems to be underused or simply forgotten: Keyword `explicit`.
How a Bug in Our Code Could Hide a Bug in the Compiler
A few days ago when I had completed a little coding at work, I recompiled the project and got a very weird compiler error in a very distant part of the code. I would never had guessed that that particular part of the code could be influenced by my changes, but somehow something I did to one of the headers changed everything on the other side of the little code-world I work in. The error was in a very simple function and looked like this:
#include <string> //more includes... namespace MyNS { bool checkSomeStrings(std::string const& a, std::string const& b) { if (a == b) //ERROR //... } } error: ambiguous call to operator == could be: operator==(Sion const&, Sion const&) or: operator==(Annie const&, Annie const&)
“Wait, what????”. I did not believe the error message. That could not be. I mean, it’s `std::string`, right? If that line can’t compile, we are in serious trouble! I changed the code just a bit, and recompiled. The error remained.
What we figured out after some time was that we had two problems: The first and really grave problem was that the compiler somehow had forgotten what ADL aka Koenig Lookup was. It simply had no idea that there was an `operator==` for strings somewhere in namespace `std` that should be used for the comparison.
The other problem that had hidden the compiler bug for some months was that one of our ancient user defined classes had an non-`explicit` one-argument constructor taking a `std::string`. Only when I included a header somewhere up in the include hierarchy that defined another equally ancient `class Sion`, the problem emerged, because that class hat a similar constructor.
What Had Happened
When there was only `Annie` and the bug did prevent the compiler from finding `operator==` for strings, it found a workaround: Non-`explicit` one-argument constructors can be used for implicit conversions, so the compiler happily converted both strings into `Annie`s and used `operator==` for `Annie` to compare those. Problem solved (for the compiler).
Only after I messed around with the headers and the definition of `Sion` was visible at that point in the code, the compiler got lost: it still did not see `operator==` for strings. But it now could do two different versions of that “workaround”, either by converting to `Annie` as before, or by converting to `Sion`. Thus the ambiguity error.
Fixing Our Bug
The first thing to do was taking the implicit conversions out of the equation, by simply adding `explicit` to the constructors of both `Annie` and `Sion`. I did some other minor refactorings once since I had touched those classes anyways and recompiled, expecting some error about the compiler finding no suitable `operator==` anymore.
class Annie { //... public: explicit Annie(string const&); };
I was wrong. It compiled cleanly, and when I debugged the function to make sure it had not found yet another “workaround”, I found it used `std::operator==` for strings, as it should do. I did not understand why the ADL bug would simply disappear, but I did not want to put too much time in more investigation. Our compiler sometimes acts weird like that.
However, one or two days later my colleague Ennox came across the ADL bug again. He had changed something in a distant header file and suddenly the compiler complained about a call to an undefined `operator==`. After some trying around that line and a few similar others in the same source file now look like this:
if (std::operator==(a,b)) { //... }
Ugh. Not very nice. Not very simple. But what else is there to do if one is stuck with a compiler like that?
Lessons Learned
What I learned from that incident was that not every developer using C++ does know or care about or remember that little feature named `explicit`. In addition this was the first time I came across a real world example where a bug had occurred because someone did not follow the basic rule about implicit conversions:
By default, don’t allow implicit conversions for user defined types. Only make them possible after careful considerations, otherwise your compiler will surprise you applying them where you least expect it.
Implicit conversions to and from user defined types are possible, if those types provide constructors that can be called with one argument, and/or if they provide conversion operators. The keyword `explicit` does forbid those implicit conversions for constructors since C++98, and for conversion operators since C++11.
class LB { public: explicit operator double(); };
So, the above rule expressed a bit closer to the coding is:
When writing a conversion operator or a constructor that can be called with one argument, make it explicit by default.
8 Comments
Permalink
cppcheck gives a warning about inexplicit, single-parameter constructors.
Permalink
Sadly, we did not have a static analyzer in place. In fact, we were still working at getting the warnings from the compiler itself to 0. At that time we had about 800 compiler warnings, many of them bogus like this one.
Permalink
Really interesting read! What compiler are you using?
Permalink
I switched jobs, so luckily I am not stuck with it aby more. It was Embarcadero’s C++ Builder 2010 which gave us lots of trouble with different compiler bugs, frequent debugger crashes in the IDE etc.
Permalink
I used to work with Borland C++ Builder ten years ago, and back then it was great. It’s sad to see what it has become.
Permalink
I was really focused on your discovery until I realized you named your variables after LoL Champions, so I stopped reading and wrote this comment.
Permalink
I’m sorry, it was not my intention to distract you from reading the post 😉
Permalink
I knew there was something familiar in the variable names…
Very interessting post anyway and definitely something to keep in the back of your head.