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

  • 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).
  • 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
  • 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.
  • 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).
  • Does the code mean E_NOTIMPL?

    In other words, indicate to the caller that the function is not implemented if the function call failed?
  • 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.
  • 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?

  • Arg, my last post was already summed-up by Skywing :(
  • 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.
  • 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.

  • James, you're on the right track. In fact, you're close enough to taste it.
  • 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.
  • #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.
  • Stab1:

    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?
  • I'm starting to sound like a broken record.

    bob, you're right. But that's not what's wrong with the code.
Page 2 of 5 (64 items) 12345