Larry Osterman's WebLog

Confessions of an Old Fogey
Common Tasks
Search
Recent Posts
Tags
Archives
Archives
Links
MSDN Blogs > Larry Osterman's WebLog > What's wrong with this code, part 10
Blog - Title

What's wrong with this code, part 10

What's wrong with this code, part 10

15 Mar 2005 10:32 AM
  • 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.

Comments
  • Skywing
    15 Mar 2005 10:46 AM
    For one, you should be using HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED).
  • Larry Osterman
    15 Mar 2005 10:51 AM
    Skywing,
    Believe it or not, that's actually NOT what's wrong.
  • Lionel Fourquaux
    15 Mar 2005 11:00 AM
    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.
  • Someone
    15 Mar 2005 11:03 AM
    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?
  • Larry Osterman
    15 Mar 2005 11:06 AM
    #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.
  • Eric K.
    15 Mar 2005 11:13 AM
    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.
  • Mike R
    15 Mar 2005 11:14 AM
    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.


  • Mike R
    15 Mar 2005 11:15 AM
    Scratch that last post... Larry Defined it as it should have been.
  • Larry Osterman
    15 Mar 2005 11:22 AM
    Eric, not really - return will exit the function.

    My last couple of comments have been huge hints. Lionel touched on the issue, though.
  • Mike R
    15 Mar 2005 11:25 AM
    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.
  • Skywing
    15 Mar 2005 11:26 AM
    FAILED will consider ERROR_NOT_SUPPORTED as a success code.

    "FAILED
    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))
    hr = ERROR_NOT_SUPPORTED;
  • Larry Osterman
    15 Mar 2005 11:27 AM
    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.
  • Larry Osterman
    15 Mar 2005 11:30 AM
    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.
  • Mike R
    15 Mar 2005 11:31 AM
    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.
  • Larry Osterman
    15 Mar 2005 11:32 AM
    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