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)) 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
- Anonymous
March 15, 2005
For one, you should be using HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED). - Anonymous
March 15, 2005
Skywing,
Believe it or not, that's actually NOT what's wrong. - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
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? - Anonymous
March 15, 2005
#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. - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
Scratch that last post... Larry Defined it as it should have been. - Anonymous
March 15, 2005
Eric, not really - return will exit the function.
My last couple of comments have been huge hints. Lionel touched on the issue, though. - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
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). - Anonymous
March 15, 2005
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 - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
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). - Anonymous
March 15, 2005
Does the code mean E_NOTIMPL?
In other words, indicate to the caller that the function is not implemented if the function call failed? - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
Arg, my last post was already summed-up by Skywing :( - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
James, you're on the right track. In fact, you're close enough to taste it. - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
#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. - Anonymous
March 15, 2005
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.
Stab2:
You didn't say if the code compiled or not ... is the compiler going to complain that goto Exit is dead code? - Anonymous
March 15, 2005
I'm starting to sound like a broken record.
bob, you're right. But that's not what's wrong with the code. - Anonymous
March 15, 2005
Someone: oops :) You're right, it's a typo. - Anonymous
March 15, 2005
mirobin,
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). - Anonymous
March 15, 2005
Right before "Exit:" I would put "hr = S_OK;" - Anonymous
March 15, 2005
Greg,
An interesting and really subtle point. It's not what's wrong, but a good point nontheless. - Anonymous
March 15, 2005
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; - Anonymous
March 15, 2005
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 - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
Michael:
#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. - Anonymous
March 15, 2005
Possibly a stupid question: is PerformAnOperation() definitely returning an HRESULT? - Anonymous
March 15, 2005
When PerformAnOpertion() actually fails and an ERROR_NOT_SUPPORTED is returned... if the caller does if(FAILED(OnSomethingHappened()) it will interpret failures as success. - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
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! - Anonymous
March 15, 2005
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 :-) - Anonymous
March 15, 2005
It doesn't actually guarantee S_OK on success.
And it gobbles up errors. Bad tester! - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
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? - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
So, if returning ERROR_NOT_SUPPORTED is correct, then the return type of HRESULT is probably wrong. - Anonymous
March 15, 2005
I feel like a newbie here- but wouldn't it make more rewrite so that the function returns a bool (true if function succeeds)? - Anonymous
March 15, 2005
But then - you lose the actual error information which is unacceptable :( - Anonymous
March 15, 2005
The comment has been removed - Anonymous
March 15, 2005
mmm...the second if always return FALSE.
# first if
if (FAILED(hr)) hr = ERROR_NOT_SUPPORTED;
#after macro expansion
if (FAILED((hr)) goto (Error) - Anonymous
March 15, 2005
Additionally, if PerformAnOperation() throws, nothing is returned from OnSomethingHappening(). - Anonymous
March 15, 2005
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. - Anonymous
March 15, 2005
...and the "Error" label is unrecheable, because the second if always returns FALSE. - Anonymous
March 15, 2005
Kyle: ERROR_NOT_SUPPORTED is a perfectly legal HRESULT - it's just not an error code (it's a success code). - Anonymous
March 16, 2005
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. - Anonymous
March 16, 2005
Actually I don't think there are any currently defined HREULTs with FACILITY_NULL. - Anonymous
March 16, 2005
Skywing. What about S_FALSE? That's an HRESULT with FACILITY_NULL - Anonymous
March 16, 2005
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. - Anonymous
June 02, 2009
PingBack from http://woodtvstand.info/story.php?id=53615