Visual C++ ternary operator considered harmful

We've recently begun upgrading our code from Visual Studio 2005 to a newer version, and we ran into the following doozy.

Imagine that you have classes with the following relationships:

// Lightweight reference to data owned by another object.
struct Light { Light(); Light(Light const&); };

// Owns data freed on release. Constructor copies data. Conversion to Light does not copy.
struct Heavy { Heavy(); Heavy(Heavy const&); Heavy(Light const&); operator Light() const; };

In a reasonable world, you would then expect the following code, using the C++ conditional operator:

Light DataOrDefault(Heavy const* h)
    { return h ? *h : Light(); }          // BAD BAD BAD!

... to work like this code using a simple if statement:

Light DataOrDefault(Heavy const* h)
    { if (h) return *h; return Light(); }       // OK

... or like this code, using a template:

template <class T> inline T Cond(bool expr, T a, T b)
    { return expr ? a : b; }

Light DataOrDefault(Heavy const* h)
    { return Cond<Light>(h, *h, Light()); }     // OK

But it does not. Do you know how it works? This is how it works.

Light DataOrDefault(Heavy const* h)
{
    // Actual behavior of: h ? *h : Light()
    Heavy t;
    if (h)
        t = *h;
    else
        t = Light();
    return t;
}

Yes. The compiler decides that the common type of the two expressions is Heavy. A temporary Heavy object is then constructed to hold this result. A copy of the data is constructed; the copy is assigned to Light; and then it is promptly destroyed, so that Light now points to invalid data.

In GCC, this produces an error. But in Visual Studio, there is no warning, even with -Wall:



Which of the two behaviors is standards-compliant, I do not know. (Added: In comments, Lee Killough argues persuasively that GCC is compliant; VC++ is not.)

On the MSDN page for the conditional operator, you can find this notice:


The compiler authors are themselves warning you this behavior is bad, and telling you not to use this operator.

Let it be clear: my concern is not making the ambiguous code work. It's to prevent it from compiling in the first place. It should at least cause a warning, so that ambiguities can be found and rooted out, and so that new ambiguities will not be implemented.

I can visually check the code, but can I write it in a way that, if it was unsafe, it would produce a warning? What if there's a conversion I didn't think of? The code would just compile silently.

Workaround?

Due to namespace issues, I dislike macros, but this lambda-based macro is the best I've come up with:

#define If(EXPR, T, A, B) \
    (([&]()> T { if (EXPR) return (A); return (B); })())

You might be surprised at the performance. Using Visual Studio 2010, in the Debug configuration, it's only slightly slower than the ternary operator. In the Release version, it is several times faster. (And I did attempt to make sure that the useful effect of the code is not being optimized away.)

Further discussion

Some argue the real danger is the implicit conversions between Heavy and Light. I find this not to be the case. The conversions are safe and intuitive in other situations. The following, for example, will not build:

template <class T> inline T Cond(bool expr, T a, T b)
    { return expr ? a : b; }

Light DataOrDefault(Heavy const* h)
    { return Cond(h, *h, Light()); }    // Compiler error: T is ambiguous

Here, the compiler observes that the template type T could be either Light or Heavy. It refuses to make a potentially unsafe and incorrect assumption, and requires the type to be specified:

Light DataOrDefault(Heavy const* h)
    { return Cond<Light>(h, *h, Light()); }     // OK

In the case of the ternary operator, MSVC simply goes ahead with one of the possible paths; often one that is very counter-intuitive. There is no error, no warning.

If we just swap the operands as follows:

Light DataOrDefault(Heavy const* h)
    { return !h ? Light() : *h; }

... the compiler decides the proper type to use is Light, and the code will work fine.


2015-06-05: Ongoing edits in response to comments.
2015-06-06: Used tohtml.com/cpp for syntax highlighting instead of images.

Comments

Philipp Lenk said…
Why exactly do you expect the compiler to know you want Light to be the common type of an expression involving Light and Heavy? You defined conversions both ways and although you mentioned in a comment that one is "better" than the other, your code does not reflect that. If a conversion from Light to Heavy is not appropriate in all situations in which it could arise it should for all intents and purposes be declared explicit. This is not the compilers or the languages fault, you do not give it the means to properly deduce the type you want. In all the "equivalent" versions mentioned abvoe the type is - in some way or another - stated explicitly. For example:

template T Cond(bool expr, T a, T b)
{
return expr? a : b;
}

You passed T directly and pass both parameters by value, ensuring they are converted even before the ternary is evalutated.
This is not a language defect, it is a defect in your own code. If both conversions are possible, neither is better than the other and you want one specific one to happen, any desicion the compiler could make on the users behalf will not satisfy everyone. It is therefore, in my humble opinion, the only sane approach to make you, the user, specify what he or she wants.
denis bider said…
Philipp: If both conversions are possible, neither is better than the other

If both conversions are possible, the code is in error, and there must be an error or warning. Note that this will not compile:

Cond(h, *h, Light());

The compiler cannot unambiguously deduce type T in Cond<T> and reports an error.

If the ternary operator was safe, it would also report such an error. You are language lawyering that the ternary operator is well defined. It is. But it's far from safe, and this is inconsistent with other language features.

See the Further discussion section.
Philipp Lenk said…
Thanks for the quick reply ;-)
You do not want the return type deduced to be Light but want a hard compiler error instead? In that case I apologize because i misunderstood and must admit you have a point.
I'd still argue the conversion should be explicit if it can ever cause any problems but i guess thats another discussion.
denis bider said…
Yes. My concern is not making the ambiguous code work, but preventing it from compiling in the first place. Or at least having it cause a warning, so that ambiguities can be found and rooted out, and so that new ambiguities will not be implemented.

At this time, I know of no way to reassure ourselves that a given use of the ternary operator, old or new, is actually safe. I am inclined to ban it and require the use of the Ternary macro, instead.
Philipp Lenk said…
Sorry to bother you again, yet i have to ask one more thing: are you entierly certain this is standard behaviour. I tested with clang 3.6 and g++ 4.9.2 and both produce an error:
(this one is from clang)
error: conditional expression is ambiguous; 'const Heavy' can be converted to 'Light' and vice versa
return h?*h:Light();
^~~ ~~~~~~~

Furthermore the standard (in $5.16) contains the following statement about it:

"[...]Using this process, it is determined whether the second operand can be converted to match the third operand, and whether the third operand can be converted to match the second operand. If both can be converted, or one can be converted but the conversion is ambiguous, the program is ill-formed.[...]"
denis bider said…
You are correct. I assumed the behavior implemented by Visual Studio is standard-compliant - which it may very well be - but GCC refuses to accept the code (it produces the exact error I would expect), and you've found the same with clang.

I have made edits to the post and title accordingly.
Lee Killough said…
Visual Studio is not standard-compliant. From the Standard:

"Otherwise, if the second and third operand have different types and either has (possibly cv-qualified) class type, or if both are glvalues of the same value category and the same type except for cv-qualification, an attempt is made to convert each of those operands to the type of the other. The process for determining whether an operand expression E1 of type T1 can be converted to match an operand expression E2 of type T2 is defined as follows:

...

"Otherwise (i.e., if E1 or E2 has a nonclass type, or if they both have class types but the underlying classes are not either the same or one a base class of the other): E1 can be converted to match E2 if E1 can be implicitly converted to the type that expression E2 would have if E2 were converted to a prvalue (or the type it has, if E2 is a prvalue).

"Using this process, it is determined whether the second operand can be converted to match the third operand, and whether the third operand can be converted to match the second operand. If both can be converted, or one can be converted but the conversion is ambiguous, the program is ill-formed."

As GCC says, your program is ill-formed, because both conversions are possible. You need to make one of them explicit or remove one of them, or use static_cast<> in the conditional expression to decide which one you want.

I seem to recall Visual Studio putting less precedence on conversion operators than conversion constructors, which is why it prefers Heavy in this case. It's simply a long-standing Visual Studio bug of it not applying conversion operators correctly and as equally as conversion constructors.

As a workaround, you could change operator Light() const in Heavy to Light(Heavy const &) in Light. I expect Visual Studio will then complain about the ambiguity.

Template argument deduction is much different than conversion rules, so using a template to deduce a single type for the conditional expression will fail if the expressions have different class types and one is not the base of the other, even if there is an unambiguous conversion from one class to the other.

The C++ language is quite well-defined on what should happen if the conversions are not ambiguous. If one class can be implicitly converted to another, and not the other way around, then the converted type is the type of the conditional operator. Visual Studio does not apply conversion operators correctly in all cases, which is why it did not flag this error and simply chose to go with Heavy.
denis bider said…
Lee,

thank you for your response. I have referenced your comment in the body of the post.

Popular posts from this blog

When monospace fonts aren't: The Unicode character width nightmare

"Unreachable" beauty standards

Is the internet ready for DMARC with p=reject?