Polymorphism and the Ternary Operator: Trickier Than You Think

Picture yourself performing a code review. The fix you're required to accept or reject looks like this:

The code before:

void myFunction()
{
    if (shouldUseBase())
    {
        Base const base{};
        base.f(g(), h());
    }
    else
    {
        Derived const derived{};
        derived.f(g(), h());
    }
}

Base is a base class of Derived, and f is a virtual overridden method:

struct Base
{
    virtual void f(int x, int y) const;
};

struct Derived : Base
{
    void f(int x, int y) const override;
};

It's inspired, almost copied, from a true story in code I've seen in a project. In the fix you're reviewing, the code of myFunction above has been changed to this:

void myFunction()
{
    Base const& baseRef = shouldUseBase() ? Base{} : Derived{};
    
    baseRef.f(g(), h());
}

The change looks nice. For one thing the code has become more concise and more expressive. Also the fix removes some duplication: the call to f() was made two times, each time by calling g() and h(). Now this is written only once.

The new code uses the technique of Herb Sutter's most important const: despite the fact that the const reference points to a temporary object, the lifetime of that object is extended until the end of the reference's lifetime.

So no worries about the reference being dangling.

It all looks good!

Do you accept the commit?

Take a moment and think about it...

...

...

...

...

Made up your mind yet?

Just look at it one more time to make sure...

The code before:

void myFunction()
{
    if (shouldUseBase())
    {
        Base const base{};
        base.f(g(), h());
    }
    else
    {
        Derived const derived{};
        derived.f(g(), h());
    }
}

The code after:

void myFunction()
{
    Base const& baseRef = shouldUseBase() ? Base{} : Derived{};
    
    baseRef.f(g(), h());
}

So?...

...

...

...

Okay, do you accept or reject the commit?

The answer is: YOU SHOULD REJECT THIS COMMIT!

Polymorphism and the Ternary Operator

Despite the fact that it seems to make code more expressive, this commit is introducing a bug: whatever value shouldUseBase() returns, baseRef points to an object of type Base, not Derived.

This means that even if shouldUseBase() returns false, baseRef points to a Base, even if the code reads Derived{}.

When I debugged that code for the first time I was quite surprised. Here is a sandbox to illustrate the problem. Press run to test this for yourself.

Not really expressive after all, right?

Polymorphism and the Stack

What's happening? Why isn't baseRef pointing to an object of type Derived?

For one thing, it's not about the "slicing problem" (see Effective C++, item 20, Prefer pass-by-reference-to-const to pass-by-value), because there is no copy involved. The reference points to the temporary object that is constructed on the line of the ternary operator.

I haven't found the legal reason behind this behavior, but this code was doomed to fail anyway. Indeed, it uses runtime polymorphism on the stack.

The choice to instantiate a Base or a Derived is made at runtime, based on the value returned by shouldUseBase(). But on the other hand, the object is allocated on the stack. And the size of the object on the stack must be known at compile time for the compiler to generate assembly code referring to it.

Put another way, the compiler needs to know the type of the object it allocates on the stack. And in our case, it chooses to allocate an object of type Base. Later on, when the program is run and shouldUseBase() returns a value, the object allocated on the stack is therefore of type Base no matter what.

It would have been nice for this code to trigger a compilation error. Indeed, I can't imagine how this code is what we want. Or at least a warning. But even with -Wall -Wextra -pedantic, gcc and Clang compile the code without a word.

Removing the Duplicated Code

Now that we've rejected this commit, isn't there a way to improve the original code without introducing the bug?

One of the issues in the original code was the duplication of the call to f and the invocation of g() and h().

To remove this duplication, we can still use the ternary operator without using the stack as in the original buggy fix. We can use the heap instead with a smart pointer to avoid worrying about writing memory management code:

auto basePtr = shouldUseBase() ? std::make_unique<Base>() : std::make_unique<Derived>();

basePtr->f(g(), h());

This code has the behavior we expect: it instantiates a Base object if shouldUseBase() returns true and a Derived object if it returns false.

The downside of this fix is that it consumes more resources because heap allocation makes calls to the operating system to obtain a space in memory and hand it back, which takes more time than allocating on the stack as in the (correct) code before any fix.

Depending on where this code is located, this slowdown can be imperceptible or can be a problem. Only profiling the program can tell.

That said, according to the 80-20 rule, only a minority of places in the codebase are critical to performance.

A Concrete Base Class: A Design Flaw

The code around our initial buggy fix has a concrete (instantiable) base class. This design is not recommended for various reasons.

If a class inherits from a concrete class, there certainly is some shared abstraction between the two, and this shared abstraction can be extracted in a common abstract base class.

If the base class was abstract and its behavior taken down to a new derived class, we wouldn't have had to review the buggy fix to begin with because the compiler would have stopped it first.

Indeed, if instead of Base and Derived we had had Derived1 and Derived2 that inherited from Base, our initial fix would have looks like this:

Base const& baseRef = shouldUseDerived1() ? Derived1{} : Derived2{};

baseRef.f();

And this code doesn't compile because the two types in the ternary operator are incompatible. Here is the compilation error message:

error: operands to ?: have different types 'Derived1' and 'Derived2'
Base const& baseRef = shouldUseDerived1() ? Derived1{} : Derived2{};

So ideally, none of this should have happened; the design shouldn't have had a concrete base class.

But if it has one, ideally the regression introduced by the fix should have been caught by a unit test.

And if there is no test around this code, knowing this curiosity of the language allows you to catch it in a code review.

You're performing code reviews, right?