C++ brain teaser

Question

I recently refactored code like this (MyClass to MyClassR).

#include <iostream>

class SomeMember
{
public:
  double m_value;

  SomeMember() : m_value(0) {}
  SomeMember(int a) : m_value(a) {}
  SomeMember(int a, int b)
  : m_value(static_cast<double>(a) / 3.14159 +
            static_cast<double>(b) / 2.71828)
  {}
};


class MyClass
{
public:
SomeMember m_first, m_second, m_third;

MyClass(const bool isUp, const int x, const int y)
{
  if (isUp)
  {
    m_first = SomeMember(x);
    m_second = SomeMember(y);
    m_third = SomeMember(x, y);
  }
  else
  {
    m_first = SomeMember(y);
    m_second = SomeMember(x);
    m_third = SomeMember(y, x);
  }
}
};


class MyClassR
{
public:
SomeMember m_first, m_second, m_third;

MyClassR(const bool isUp, const int x, const int y)
: m_first(isUp ? x : y)
, m_second(isUp ? y : x)
, m_third(isUp ? x, y : y, x)
{
}
};


int main()
{
    MyClass a(true, 1, 2);
    MyClassR b(true, 1, 2);

    using namespace std;
    cout.precision(10);
    cout
        << "a:" << endl
        << "\tfirst: " << a.m_first.m_value 
        << "\tsecond: " << a.m_second.m_value 
        << "\tthird: " << a.m_third.m_value << endl;

    cout
        << "b:" << endl
        << "\tfirst: " << b.m_first.m_value
        << "\tsecond: " << b.m_second.m_value
        << "\tthird: " << b.m_third.m_value << endl;

    return 0;
}

• What is the error,
• why does it compile (tested with VC6 as well as VC9 warning level 4: no complaints) and
• what is the right way of doing it?

I (assume) I already have all these answers but I think it's and interesting problem to share.

Update
Extended code so it's "copy & paste & execute"-able. VC9 gave me no complaints either so VC6 is not the problem here.
For completeness, the output is:

a:
        first: 1        second: 2       third: 1.054069532
b:
        first: 1        second: 2       third: 1.004499999

Answer 1

I’m not sure what exactly you expect but let’s start …

• First off, ditch VC6. Seriously. Using it is a huge problem since it’s just not standards conforming and precludes a lot of options. Using it correctly is like playing Russian roulette.

• Your constructor of m_third doesn’t do what you think it does. You cannot write a conditional expression like this: “several parameters” is not a valid expression in C++, and the conditional operator works on expressions.

• The code compiles because it’s still correct, it just doesn’t do what you want it to. Instead of using “several parameters”, it evaluates the sequence point operator (,) which just takes the last value of the expression, so your conditional is effectively equivalent to: isUp ? y : x

• The right way is to use two conditionals: m_third(isUp ? x : y, isUp ? y : x)

• The third constructor of SomeMember is wrong, the value may overflow, yielding a negative value – I highly doubt that that’s what you want.

Answer 2

m_third(isUp ? x, y : y, x)

This looks wrong to be. The first x is a pointless expression as it has no side effects and the result is not used, then the two sides of the : have the same value and side effects so ?: can be elimintated as the expression before the ? also has no side effects.

m_third(y, x)

But now it doesn't do what the original code does... is this the error?

Answer 3

What is the error what is the right way of doing it?

I guess your intention is to show some kind of naive usage of comma operator in combination with ternary ?, perhaps there is some clever and unexpected operator priority gotcha hidden, but that I think the code is absolutely artificial. If this is the point, than I would say the "right way of doing it" is do not use C++ or first learn it before using it. Yes, it has many constructs which may look like "quirks" and you can create a lot of strangely looking code accepted by a compiler. By using C++ I would say you are assumed to know the tools.

Why is does compile

Because it contains no error and it is a correct C++ code with no ambiguities.