Aaargh! Part Two

Rate This

Eric Lippert

23 Mar 2004 10:58 AM

Comments 12

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

COM Programming, Rants, Aargh!, bad jokes

Brian

23 Mar 2004 1:23 PM

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!
Eric Lippert

23 Mar 2004 1:45 PM

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.
Shane King

23 Mar 2004 3:57 PM

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

23 Mar 2004 7:02 PM

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.
Frederik Slijkerman

24 Mar 2004 12:25 AM

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.
Kim Gräsman

24 Mar 2004 1:34 AM

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-lover
Misha

26 Mar 2004 9:14 PM

Sounds so familiar! I was banging my head against the wall trying to understand why GetErrorInfo returns an empty object after I implicitly called SetErrorInfo in the previous function call. It turned out I had a smart pointer to a cross-process object! Calling Release() cross-process is associated with entering the message loop which results in clearing IErrorInfo pointer on the thread local storage. And I needed a help of a very smart guy to hunt it down.
Will I stop using smart pointers? Not yet, but from now on I will watch out for this, until it hits me again.
Tirion

5 Jun 2004 8:33 AM

[ Waste Book ] 04-06-05
Fabulous Adventures In Coding

14 Jun 2004 2:52 PM
MuKo

30 Dec 2005 11:45 AM

Writing the Get|Set<PropertyName> as a macro makes the C programmer's and C maintainer's life easier (gripe #3). The alternative in C++ would be a template. Some programmers like to think outside the box.
Yuhong Bao

19 Jul 2007 8:17 PM

Why not do this:

int ffddf()

{

{

SmartPointer SmartPointer;

} <-- This will call the destructor

SetLastError()

return 0;

} <-- no-op
Markus Schaber

27 Jul 2009 5:49 AM

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.

Aaargh! Part Two

Eric Lippert’s Blog

A new fabulous adventure

Why is deriving a public class from an internal class illegal?

It's still essential!

Dynamic contagion, part two

Aaargh! Part Two