What’s wrong with this code, part 10

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))
    IF_FAILED_JUMP(hr, Error);

    return hr;

    goto Exit;

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

As usual, answers and kudos tomorrow.

Comments (64)

  1. Anonymous says:

    For one, you should be using HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED).

  2. Anonymous says:


    Believe it or not, that’s actually NOT what’s wrong.

  3. Anonymous says:

    As Skywing, I think HRESULT_FROM_WIN32 should be used: ERROR_NOT_SUPPORTED is a Win32 error code, not an HRESULT, and I don’t remember anything in the documentation that ensure that FAILED(ERROR_NOT_SUPPORTED) is true.

    The initial comment states that S_OK is returned for success. This could be true (depending on the possible return values of PerformAnOperation), but this code doesn’t ensure it: if FAILED(hr) is false (even for a warning code), it return hr as is.

  4. Anonymous says:

    I’m dying to know what IF_FAILED_JUMP is. Does it throw an exception? Does it free memory? Does it display a dialog? Does it change hr?

  5. Anonymous says:

    #define IF_FAILED_JUMP(hr, label) if (FAILED((hr)) goto (label)

    That’s it, nothing special.

    Lionel, I also thought as Skywing thought. But I was wrong.

  6. Anonymous says:

    Could it be that, after returning the exit value, execution will continue through to the Error label and loop infinitely?

    I don’t program in this language, so I’m not sure if the return statement unconditionally exits or not, but it looks like it’s begging to do so.

    It also seems to me that, error or not, you’ll end up in the same place: At the return statement.

  7. Anonymous says:

    Rummaging through past posts, I came across this definition:

    #define IF_FAILED_JUMP(hr, tag) if ((hr) != S_OK) goto tag

    HRESULTS are successful, when positive, they are only failure codes when negative (high bit set, SEVERITY_ERROR, I believe, when passed into the MAKE_HRESULT macro. )

    This assumes the function fails if the HRESULT is not S_OK.

  8. Anonymous says:

    Scratch that last post… Larry Defined it as it should have been.

  9. Anonymous says:

    Eric, not really – return will exit the function.

    My last couple of comments have been huge hints. Lionel touched on the issue, though.

  10. Anonymous says:


    ERROR_NOT_SUPPORTED doesn’t seem to be an appropriate error code, but maybe that’s not material to the problem.

    Is it save to assume that PerformAnOperation does return an HRESULT?

    One thing that strikes me as odd is this whole goto Exit, which simply returns back to the previous line. It seems unnecessary.

  11. Anonymous says:

    FAILED will consider ERROR_NOT_SUPPORTED as a success code.


    Provides a generic test for failure on any status value. Negative numbers indicate failure.

    #define FAILED(Status) ((HRESULT)(Status)<0)"

    I don’t see how mixing up Win32 error encodings and HRESULT error encodings can possibly be correct in this (or any) situation.

    As a result the IF_FAILED_JUMP will never execute. Anything that was really a failure HRESULT will be turned into a success HRESULT by this:

    if (FAILED(hr))


  12. Anonymous says:

    Yes, PerformAnOperation DOES return a valid HRESULT.

    The goto Exit thingy is actually done that way because it means that the non error termination clause is straight line and the error clause is visually tagged as being out-of-line. It’s a style thing, more than anything.

  13. Anonymous says:

    Aaah, you’re getting extremely close to the answer Skywing..

    Remember – this is exactly (except for renaming code and stripping out a bunch of stuff that’s non germane) the code that I encountered (it’s in a test app, so it’s not a part of windows).

    But the HRESULT vs Win32 error code issue is NOT the issue.

  14. Anonymous says:

    One more stab:

    ERROR_NOT_SUPPORTED is a system error, and not a proper HRESULT, returning this error to the caller has the potential to trip-up a caller, which assumes, because it is not a failed code that the function call succeeded, even though it didn’t.

  15. Anonymous says:

    Mike, you’re totally correct. But that’s not what’s wrong with the function.

    I’m sorry, I can’t give more detailed hints than what I’ve given without giving the answer away.

  16. Anonymous says:

    If FAILED only returns true on negative results then by changing the error condition into ERROR_NOT_SUPPORTED you have changed it into a success condition (from FAILED’s point of view).

  17. Anonymous says:

    I seem to remember in gcc that there was an issue with this kind of construct not calling the destructor at all for all objects created in the function.

    Just guess though

  18. Anonymous says:

    Actually, because you set hr = ERROR_NOT_SUPPORTED, which as others have already said is treated as a success code by the FAILED macro, it doesn’t take the IF_FAILED_JUMP path even in the failure case. It just executes the next statement afterward.

  19. Anonymous says:

    In either case, that IF_FAILED_JUMP clause doesn’t make any sense here.

    You’re already checking if the function failed before, and if it did fail then you’re changing it to some constant value and then retesting that constant value.

    As it is right now, due to the error encoding issue this function will never return a proper failure status, ever (it will always pass SUCCEEDED(hr) for hr=CThing1::OnSomethingHappens()).

    Even if that wasn’t the issue, you’re losing information (unnecessarily?) by changing any failure status into (what I assume is supposed to be, even if it’s not really right now) a specific "not supported" failure status.

    The function is documented to generalize all success codes into S_OK, but what it actually does is generalize al failure codes into ERROR_NOT_SUPPORTED (which is again technically a success code, but we’ll ignore that since you said to do so).

  20. Anonymous says:

    Does the code mean E_NOTIMPL?

    In other words, indicate to the caller that the function is not implemented if the function call failed?

  21. Anonymous says:

    IANAMP (I am not an MFC Programmer) but my flailing guess would look at why the statement

    if (FAILED(hr))

    can ever prove a universal negative instead of compairing against S_OK which is the only valid result. But that’s not a symptom of using the proper API calls, so I doubt that’s the answer.

  22. Anonymous says:

    If PerformAnOperation fails, hr will then be ERROR_NOT_SUPPORTED, which is positive.

    If PerformAnOperation succeeds, hr will be >= 0, assuming PerformAnOperation’s HRESULT return behavior is standard.

    Looking at the definition of FAILED:

    #define FAILED(Status) ((HRESULT)(Status)<0)

    We can see that the IF_FAILED_JUMP and the following labels useless, since the IF_FAILED_JUMP will never occur (hr will always be positive or 0 [S_OK]).

    So is the problem that the code will never execute?

  23. Anonymous says:

    Arg, my last post was already summed-up by Skywing 🙁

  24. Anonymous says:

    Is the problem that the function is documented to return only S_OK on success? If PerformAnOperation returns S_FALSE or something, it’s not clear whether that’s supposed to be a success or a failure, and it’s not converted. Of course, I’m not sure what the jumping and pointless flow control macros are all about and can’t see why you’re not interested in the HRESULT_FROM_WIN32 issue, so I don’t really know.

  25. Anonymous says:

    Folks, all of your comments are absolutely true (I’m not sure about MikeR’s or the GCC issue).

    But there’s still something wrong with the code.

    I REALLY, REALLY meant it when I said it was tricky.

  26. Anonymous says:

    James, you’re on the right track. In fact, you’re close enough to taste it.

  27. Anonymous says:

    The problem i see is that you lose the actual failed hr result from the PerformAnOperation() function. It doesn’t let the caller know the actual failure.

  28. Anonymous says:

    #define IF_FAILED_JUMP(hr, label) if (FAILED((hr)) goto (label)

    This has unbalanced parenthesis, but I bet that’s a typo and not what’s in the code.

  29. Anonymous says:


    MSDN says: A jump-statement must reside in the same function and can appear before only one statement in the same function

    The last half of that sentence is someone confusing, but it sounds like it is saying that a goto can’t be the last statement in the function.


    You didn’t say if the code compiled or not … is the compiler going to complain that goto Exit is dead code?

  30. Anonymous says:

    I’m starting to sound like a broken record.

    bob, you’re right. But that’s not what’s wrong with the code.

  31. Anonymous says:

    Someone: oops 🙂 You’re right, it’s a typo.

  32. Anonymous says:


    A goto can be the last statement, that’s just fine. And the compiler doesn’t care about the dead code issue – it just silently optimizes it away (which is good).

  33. Anonymous says:

    Right before "Exit:" I would put "hr = S_OK;"

  34. Anonymous says:


    An interesting and really subtle point. It’s not what’s wrong, but a good point nontheless.

  35. Anonymous says:

    I got it!

    PerformAnOperation() could return S_FALSE to mean "Success". However, the way this function is crafted, the caller would assume that S_FALSE is a "Failure" code. In fact, S_FALSE would be treated the same as ERROR_INVALID_FUNCTION.

    What needs to happen is this:

    if (SUCCEEDED(hr)) hr = S_OK;

  36. Anonymous says:

    Someone: This is SOOO painful. You guys are SO close, but that’s not it.

    S_FALSE is a great, subtle point (essentially it’s the same point that Greg Wishart made just above).

    But it’s not the problem with this code.

    Let’s consider what I’ve said above:

    #1: This is a tricky problem (this is IMPORTANT).

    #2: hr = HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED) is NOT the error. That part of the code is correct.

    Ask yourselves "Under what circumstances would #2 be correct?".

    Here’s another (subtle) hint: http://blogs.msdn.com/larryosterman/archive/2004/11/09/254561.aspx

  37. Anonymous says:

    I think its the following issues:

    1) The function calls PerformAnOperation and upon its failure blindly converts the failure into success by setting it to ERROR_NOT_SUPPORTED. (This doesn’t indicate the E_NOTIMPL, but rather the operation is not allowed in the current state of the object/system/function whatever PerformAnOperation does.) ERROR_NOT_SUPPORTED is however a Win32 positive error code, which by OLE/COM conventions is a success code.

    2) OnSomethingHappening is documented as to return S_OK only, which is wrong: The implementation additionally returns ERROR_NOT_SUPPORTED and whatever success codes are returned from PerformAnOperation. (e.g. S_FALSE and others)

    3) In fact the documentation of at least one error state (ERROR_NOT_SUPPORTED) is missing. This must appear in the documentation block!

    4) As noted already the IF_FAILED_JUMP is unnecessary, but the compiler would remove it.

    Basically the main problem lies in the handling of the result of PerformAnOperation and the documentation of the OnSomethingHappening function itself. If a caller relies on the function specification (that it returns S_OK only), the caller could assume it will always succeeds and not do an error check at all – which could in the worst case destabilize a whole component/process.

    The missing documentation and wrong use of HRESULT values are key to this post – my thoughts.

  38. Anonymous says:


    #1 is not a problem, the code that converts the failure to ERROR_NOT_SUPPORTED is correct.

    #2: This is SOOO close. It’s the closest anyone’s been so far.

    #3: Absolutely.

    #4: Yup.

    I’m going to declare victory for Michael – he nailed what is the crux of the problem: The semantics of returning ERROR_NOT_SUPPORTED are not spelled out anywhere in the routine, even though the semantics are important.

    And the absence of that documentation caused a very real bug that took one of my peers the better part of a day to track down last week. More tomorrow.

  39. Anonymous says:

    Possibly a stupid question: is PerformAnOperation() definitely returning an HRESULT?

  40. Anonymous says:

    When PerformAnOpertion() actually fails and an ERROR_NOT_SUPPORTED is returned… if the caller does if(FAILED(OnSomethingHappened()) it will interpret failures as success.

  41. Anonymous says:

    I’m coming in way late here, sorry if I’m repeating anything, but the big problem is that if OnSomethingHappening() encounters an error along the way (for example PerformAnOperation() fails), then it returns a *successful* HRESULT.

    Since the caller (hopefully) tests the retval of OnSomethingHappening() with the SUCCEEDED/FAILED macros, the caller has no way to know that OnSomethingHappening() failed.

    If the caller is testing against S_OK explicitly, then this code is misusing HRESULTs.

  42. Anonymous says:

    I should expand upon that:

    HRESULT is just a typedef for LONG, so if PerformAnOperation returns a standard win32 error (or some arbitrary DWORD) then those macros could be confused.

    PerformAnOperation might even have a failure result that == S_OK!

  43. Anonymous says:

    This post reminds me of my software engineering class in college: Software development is not only coding. Which again reminds me to take a closer look at my code and its documentation 🙂

  44. Anonymous says:

    It doesn’t actually guarantee S_OK on success.

    And it gobbles up errors. Bad tester!

  45. Anonymous says:

    Just my opinion but I think the problem of treating ERROR_NOT_SUPPORTED as an HRESULT when it is not, is a much bigger deal than the documentation of the function.

    If the real problem were corrected, the "problem" you were looking for would not have been an issue.

    Just my two cents.

  46. Anonymous says:

    I’ll take a stab – why are you changing the return code at all? you are hiding the true error that occurred in PerformAnOperation

  47. Anonymous says:

    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?

  48. Anonymous says:

    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.

  49. Anonymous says:

    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.

  50. Anonymous says:

    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())


    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.

  51. Anonymous says:

    So, if returning ERROR_NOT_SUPPORTED is correct, then the return type of HRESULT is probably wrong.

  52. Anonymous says:

    I feel like a newbie here- but wouldn’t it make more rewrite so that the function returns a bool (true if function succeeds)?

  53. Anonymous says:

    But then – you lose the actual error information which is unacceptable 🙁

  54. Anonymous says:

    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.

  55. Anonymous says:

    mmm…the second if always return FALSE.

    # first if


    #after macro expansion

    if (FAILED((hr)) goto (Error)

  56. Anonymous says:

    Additionally, if PerformAnOperation() throws, nothing is returned from OnSomethingHappening().

  57. Anonymous says:

    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.

  58. Anonymous says:

    …and the "Error" label is unrecheable, because the second if always returns FALSE.

  59. Anonymous says:

    Kyle: ERROR_NOT_SUPPORTED is a perfectly legal HRESULT – it’s just not an error code (it’s a success code).

  60. Anonymous says:

    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.

  61. Anonymous says:

    Actually I don’t think there are any currently defined HREULTs with FACILITY_NULL.

  62. Anonymous says:

    Skywing. What about S_FALSE? That’s an HRESULT with FACILITY_NULL

  63. Anonymous says:

    3/16/2005 7:20 AM Larry Osterman

    > Kyle: ERROR_NOT_SUPPORTED is a perfectly

    > legal HRESULT – it’s just not an error code

    > (it’s a success code).

    Then it should be called S_NOT_SUPPORTED just like S_FALSE.

    Calling it ERROR_NOT_SUPPORTED makes it look like ERROR_SUCCESS, lending support to the impression that all errors are successes and all WIN32 success codes are COM success hresults.