Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 10

What's wrong with this code, part 10

  • Comments 64
Ok, time for another "what's wrong with this code".  This one's trivial from a code standpoint, but it's tricky...

// ----------------------------------------------------------------------
// Function:
// CThing1::OnSomethingHappening()
//
// Description:
// Called when something happens
//
// Return:
// S_OK if successful
//
// ----------------------------------------------------------------------
HRESULT CThing1::OnSomethingHappening()
{
    HRESULT hr;
        :

        :
    <Do Some Stuff>
        :
   
    // Perform some operation...
    hr = PerformAnOperation();
    if (FAILED(hr))
        hr = ERROR_NOT_SUPPORTED;
    IF_FAILED_JUMP(hr, Error);

Exit:
    return hr;

Error:
    goto Exit;
}

Not much code, no?  So what's wrong with it?

As usual, answers and kudos tomorrow.

  • I'll take a stab - why are you changing the return code at all? you are hiding the true error that occurred in PerformAnOperation
  • OK, maybe I am missing something here, but.. this is totally wrong:

    1) You called some function and got back an HRESULT hr. Good.
    2) You tested if the function failed by calling if (FAILED(hr)). Good.
    3) Based on that, you set hr to ERROR_NOT_SUPPORTED. Bad! First of all, you've just lost the actual error information, and replaced it with a generic. Second, unless I am mistaken, ERROR_NOT_SUPPORTED is a *success* error code, since it is positive number. So you just returned a success in response to a failure.

    Am I totally off my rocket here, or what?
  • To clarify, ERROR_NOT_SUPPORTED is being _interpreted_ as a success error code, because its value is positive. It wasn't intended to be used as an HRESULT, and shouldn't be tested with SUCCEEDED/FAILED macros.
  • Doh, that was one of the first things I noticed, bad documentation. I just didn't say anything. My brother use to get onto me all the time about only documenting success returns.
  • Is the problem that there is NO WAY this function can really return an error condition?

    If everything goes well, the function returns S_OK or some positive HRESULT. Anything that goes wrong is going to result in a positive HRESULT.

    As a relatively unseasoned COM programmer, I'm likely to write this:

    if (FAILED(OnSomethingHappening())
    HandleImportantError();

    Now my error handler will never get called.

    The only real way to check for an error is to test against the value (whatever it is) of ERROR_NOT_SUPPORTED, but that might collide with some other important result.
  • So, if returning ERROR_NOT_SUPPORTED is correct, then the return type of HRESULT is probably wrong.
  • I feel like a newbie here- but wouldn't it make more rewrite so that the function returns a bool (true if function succeeds)?
  • But then - you lose the actual error information which is unacceptable :(
  • Well the thing about this code that would really piss me off had I written the PerformAnOperation method and someone else had called it, is that I might have provided some nice error information in hr and via ISupportErrorInfo. Instead the caller has just thrown it away by assigning the error code to ERROR_NOT_SUPPORTED, which is not even a proper HRESULT error value.
  • mmm...the second if always return FALSE.


    # first if
    if (FAILED(hr)) hr = ERROR_NOT_SUPPORTED;

    #after macro expansion
    if (FAILED((hr)) goto (Error)

  • Additionally, if PerformAnOperation() throws, nothing is returned from OnSomethingHappening().
  • Stewart: If PerformAnOperation throws, then clearly the caller's supposed to handle it, not OnSomethingHappened().

    Tom M: You're assuming that error codes are universally valid. They're not always - sometimes it's appropriate to map from one to another, depending on situation.

    Sriram: Again, sometimes it's ok to lose error information.

  • ...and the "Error" label is unrecheable, because the second if always returns FALSE.
  • Kyle: ERROR_NOT_SUPPORTED is a perfectly legal HRESULT - it's just not an error code (it's a success code).
  • The function is documented to return S_OK on success, so strictly speaking, the caller should check if the return value is S_OK to determine failure or success.

    But PerformAnOperation can return a success code that's not S_OK, so FAILED(hr) is false, and that hr returned to the caller, and the caller will think the function has failed.


Page 4 of 5 (64 items) 12345