Aaargh! Part Two
I'm insanely busy prepping for VSLive! today, so, once more, I complain about coding practice that are drivin' me nuts.
But first: why do pirates dislike Johnny Depp's Oscar-nominated performance in "Pirates of the Caribbean"?
Because it wasn't AAAAAARRRR-rated.
Gripe #3: More Complaining About Bad Macros
One day I was debugging a bunch of macro-ridden code that I didn't write. I wanted to know what the method signature of
CForm::GetMouseIcon was. So, silly me, I grepped the source code for CForm::GetMouseIcon. Nothing. Must be defined in the class header itself! So I grepped for GetMouseIcon. Nothing. Aaargh! It was drivin' me nuts!
Inspiration hit -- I grepped for
MouseIcon and found in some header somewhere
DEFINE_DERIVED_GETSET_METHODS(CForm, CServer, MouseIcon, IID_IPicture)
Aaargh! I still didn't know what the signature was until I managed to find that macro definition and decode it! Why do people write code like this? You can't easily debug into
CForm::GetMouseIcon, you can't find it easily using grep, etc. Why define this stuff in macros? Does it really make anyone's life easier?
Gripe #4: Exception Handling, Return Values and Smart Pointers
You probably knew I couldn't actually not complain about smart pointers,
the bane of my existence. I'm so predictable.
Before I start complaining about exception handling, please, I don't want to have a replay of that Joel On Software debate about the merits of structured exception handling. I think exception handling is super. I love writing code in C# that uses exception handling.
catch allows you to isolate your error handling code into strictly scoped blocks, and finally allows you to isolate your cleanup code, and that's all good.
The problem I have with structured exception handling is that programs that mix libraries that use SEH and libraries that return error codes, (like, say, every single COM API) often turn into a godawful mess. You end up with the worst of both worlds: exceptions potentially going off all over the place acting as non-local gotos, and still having to pay careful attention to error return codes, thereby de-localizing the error handling code. C# and the .NET Framework were designed to use exception handling from day one, so it makes a lot of sense. COM was not!
As we saw
last time, the tempting thing to do is to wrap every error-returning function with a macro that turns it into a throw, and wrap every must-be-released handle with a smart object that will clean up when it goes out of scope. But the resulting complexity means that often the cure is worse than the disease. We've already seen the havoc that macros wreak. What about smart pointers?
Case in point -- one of my coworkers and I were debugging a weird problem in some of our code just last week. We called into a particular API which was setting the
GetLastError value. Something in our code was blowing it away, so we wrote some code to preserve it. It didn't work; when our function returned, GetLastError was still reporting that there had been no error. How could that be? What's wrong with this picture?
// Ensure that we've preserved the error state we captured earlier
SetLastError(blah);
return hr;
}
The caller of this method calls GetLastError , but the state we just set has been blown away. OK, smart people, where does that happen?
Turned out that a smart pointer in the current scope was holding on to a proxy for an out-of-process COM object, and when the smart pointer went away, the out-of-proc object was released, which resulted in a whole slew of calls behind the scenes, which resulted in the
GetLastError value being overwritten. Of course, since this was a smart pointer destructor doing all this, the source code for the call site looked like this:
} <-- calls destructor
Aargh! It's drivin' me nuts! Call me crazy, but I want operations bearing complex semantics and causing side effects that break my error handling to look a little more like function calls and a little less like punctuation. Had the code been written using dumb pointers, no problem
// Ensure that we've preserved the error state we captured earlier
SetLastError(blah);
pProxy->Release();
return hr;
} <-- no op
Now it's obvious where the mistake is.
COM programming demands discipline and a keen understanding of how to write correct error-path code whether you also use exception handling or not. Therefore, I say don't use exception handling if you're going to be doing a whole lot of COM-style error handling too! I like to think that I'm a fairly bright guy, but I have great difficulty analyzing the semantics of code that has macros, template classes, smart pointers, exception raisers, error return values and cross-process COM calls all in the same routine. I'm not that smart!
I've developed a coding style that works very well with COM -- it is verbose, it is rigid, and it is very, very clear to even novice programmers exactly what the lifetime of all pointers in the code are -- and my code rarely has reference leaks because of it, even in error cases. And when it does, they are a snap to track down because every time a pointer is addrefed there is a call to AddRef and every time it is released there is a call to Release right there in the code, not tucked away in some template library. It's not hard to develop a good coding standard.
Next time: even more on bad interactions between exception handling and memory management
Comments
Anonymous
March 23, 2004
Again, your problem isn't really with smart pointers. This time it's with implicit destruction of stack objects in general. The code calling SetLastError could have been called by any stack object destructor!Anonymous
March 23, 2004
Correct -- my problem is with implicit destructor semantics in C++. I don't like 'em.
99.99% of the times that I encounter an object with an implicit destructor, it's a smart pointer.Anonymous
March 23, 2004
The promise of C++ is you can just use the parts you need.
The reality is that unless you use all of it, or none of it, you end up in an awful mess.
The problem with your strategy is that if you every have to use anything that DOES throw exceptions (such as the stl), your manual error cleanup code has just got a great deal more complicated.
I think the problem is COM. It just doesn't belong in most of your C++ code. Sure, consume COM objects under the covers, and expose the edges of your code via COM if you like. But it should mostly be kept locked out of sight of the mainline C++ code. This frees the majority of the code to use sane coding conventions, which include proper exception handling!
This is basically what the CLR is doing for C# anyway. You deal with the pretty face that has been put on top of COM and Win32 APIs. As you note, it's much nicer to work that way.Anonymous
March 23, 2004
So, could you give example for programmers of the good coding practice? It's useful to learn which practices are bad, but it's even more useful and fun to learn the good coding style, especially from the point of view of the person who has complaints.
Thanks.Anonymous
March 23, 2004
Maybe the real problem is that you try to return an error code through the Win32 API GetLastError in a C++ function. That looks like bad design to me. Why not let the C++ function throw an exception, or return an error code? That would fix the whole mess.Anonymous
March 23, 2004
Frederik,
I absolutely agree. There already is a good old HRESULT going back from the function (return hr;), why not just do
return HRESULT_FROM_WIN32(blah);
instead of
SetLastError(blah);
(assuming blah belongs in the Win32 facility)
The thing with smart pointers is that they are often considered the bane of memory/resource leaks, but in fact they are not, until you learn how they work. Much like any resource management :)
But I agree with Eric - stuff that happens implicitly with side effects is a bad thing.
/Smart-pointer-loverAnonymous
March 26, 2004
The comment has been removedAnonymous
June 05, 2004
[ Waste Book ] 04-06-05Anonymous
December 30, 2005
The comment has been removedAnonymous
July 19, 2007
Why not do this: int ffddf() { { SmartPointer SmartPointer; } <-- This will call the destructor SetLastError() return 0; } <-- no-opAnonymous
July 26, 2009
I think that your main Problem is that you use global Variables as return values as you do in SetLastError(). Just see which trickery glibc needs to make errno threadsafe, and you'll know how evil it is. There are several inventions in most modern languages, and each of them makes SetLastError & co obsolete:
- structs / tuples as return parameters
- exceptions
- out parameters / call by reference The only excuse for using global variables to return results is when you use ancient C libraries, and then you best wrap those libs using well-designed functions.