views:

84

answers:

4

I just spent way too long trying to diagnose why, in the following snippet of code, the ProcessEvent() method seemed to be ignoring the false value I passed in for aInvokeEventHandler:

HRESULT 
CEventManager::
Process(Event anEvent)
{
    return (m_pPool->GetFsm()->ProcessEvent(anEvent), false);
}

// Definition of ProcessEvent()
HRESULT ProcessEvent(const Event& anEvent, bool aInvokeEventHandler = true);

Whenever I broke in the ProcessEvent() method, aInvokeEventHandler would always be true, regardless of whether I passed in false.

It took a workmate to point out to me that the false value should be inside the inner parentheses on the return line, like so:

return m_pPool->GetFsm()->ProcessEvent(anEvent, false); // Corrected code

As soon as I saw this, I kicked myself. Spotting this was obviously made harder because the original coder used redundant outer parentheses on the return line.

My question is, why didn't the compiler pick this up for me?

My method is returning a HRESULT, yet in the original code above, I am clearly returning a composite set of values in parentheses, i.e:

(HRESULT, bool)

Is notation like this acceptable in the C/C++ standards, and if so, what purpose would there be in allowing this? Or is this a bug in the compiler?

+1  A: 

You are suffering from the comma operator, which evaluates and discards the value of its left-hand operand, and then evaluates its right-hand operand as the value of the expression.

Also, the default value for the argument to ProcessEvent is why your one-argument call was acceptable.

Jonathan Leffler
So what you are saying is that we are returning 'false' (bool type) from the Process() method in the original code, when we should be returning a HRESULT (long type)? Would any level of compiler warnings have picked this up, or is the bool->long cast considered a valid implicit conversion at all warning levels?
LeopardSkinPillBoxHat
The compiler should not complain about legitimate code, and what you wrote is legitimate. As noted in my addition to my answer (while you were typing up your question), the default argument to ProcessEvent() additionally confused you.
Jonathan Leffler
Not sure if that answered my question. I'm still returning 'false' from Process() (according to the comma operator precedence), when the method signature requires I return a HRESULT. This is an implicit bool->HRESULT(long) conversion. Shouldn't it have appeared as a warning at least?
LeopardSkinPillBoxHat
Oh - I see what you're saying; sorry. Yes, you are returning false from Process(). I don't know exactly what type HRESULT maps to, but the chances are that false (equivalent to zero) is convertible to HRESULT. Since it is a lossless transform, it is not clear that the compiler should issue a warning. If you prod it hard enough (use enough warning options), you might manage to get the compiler to give you a warning, but you'd have push it fairly hard, I think.
Jonathan Leffler
HRESULT is a DWORD which is an unsigned integer. You can convert false to the unsigned value 0 without loss, so it is unlikely that any compiler would ever warn you, no matter what setting you applied
1800 INFORMATION
A: 

The comma is actually a valid operator - see this post.

Matthew Iselin
+1  A: 

What you wrote:

return (m_pPool->GetFsm()->ProcessEvent(anEvent), false);

What it means (roughly):

bool temp = false;
m_pPool->GetFsm()->ProcessEvent(anEvent);
return temp;
1800 INFORMATION
The 'temp' is unnecessary; you could simply call ProcessEvent and return false.
Jonathan Leffler
A: 

The problem you are seeing lies in two things.

  1. The comma operator is throwing away the value on the left side and returning only the right side value (fasle)

  2. An HRESULT is just a long value, and therefore there is an implicit conversion of the value false to HRESULT, thus no compiler error.

heavyd