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.

  • For one, you should be using HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED).
  • Skywing,
    Believe it or not, that's actually NOT what's wrong.
  • 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.
  • 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?
  • #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.
  • 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.
  • 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.

  • Scratch that last post... Larry Defined it as it should have been.
  • Eric, not really - return will exit the function.

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

    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.
  • 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))
  • 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.
  • 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.
  • 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.
  • 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.
Page 1 of 5 (64 items) 12345