What's wrong with this code, part 13 - the answers
I loved this particular "What's wrong" because it was quite subtle (and real world).
One of the developers on the audio team came to me the other day and complained that the assert in CFooBase::~CFooBase was firing. I looked at the source for a few minutes, and all of a sudden realized what was happening.
You see (as mirobin figured out), an object that derives from ATL::CComObjectRoot doesn't actually implement its IUnknown. Instead, the IUnknown implementation comes from a wrapper class, the CComObject.
As a result, the IUnknown implementation in the CFooBase is dead code - it will never be called (unless someone instantiates a CFooBase directly). But, since CFooBase implements IFooBase, it also needs to implement IUnknown (even if the implementation is never called).
What complicates this scenario is that you don't get to see the constructor for the CFooDerived class - that's hidden inside:
OBJECT_ENTRY_AUTO(CLSID_FooDerived, CFooDerived)
This macro invokes linker magic to cause the class factory for the CFooDerived to instantiate the CComObject that wraps the CFooDerived - you never see the code, so it's not at all obvious where the class is being instantiated.
So for the kudos:
First off, mad props to mirobin for initially catching two huge, massive typos (ok, stupid omissions) in the original post (I forgot the AddRef in the QI method and forgot to return a value in the Release() method).
mirobin correctly called out one other issue. Instead of:
if (iid == IID_FooBase) { AddRef(); *ppUnk = reinterpret_cast<void *>(this); }
It should be:
if (iid == IID_FooBase) { AddRef(); *ppUnk = reinterpret_cast<void *>(static_cast<IFooBase>(this)); }
The reason for this is that if CFooBase implements multiple interfaces, each interface has its own vtable. So you need to do the double cast to resolve the ambiguity. The good news is that the compiler will complain if there's an ambiguous cast, which forces you to resolve the ambiguity. In this case it's not necessary, but in general it's a good idea.
Other comments: Michael Ruck complained that CFooBase() isn't implemented. You don't have to have a constructor on a class, it's possible that this class doesn't need it.
And Michael complained that ~CFooBase() is private. This one merits an entire blog post by itself.
Comments
- Anonymous
June 30, 2005
I actually meant the interface method IFooBase::FooBase() not being implemented ;)
I'm looking forward to your C++ destructors post. - Anonymous
June 30, 2005
"And Michael complained that ~CFooBase() is private." So does Visual Studio:
error C2248: 'CFooBase::~CFooBase' : cannot access private member declared in class 'CFooBase' - Anonymous
June 30, 2005
Making the destructor private is good if you have reference counting instead of conventional creation and destruction semantics.
What it does is allows the compiler to enforce that the object can only be destroyed by member functions.
If you think about this for a moment, then you'll see why this is a good idea:
- The compiler won't let you declare a reference counted object on the stack. This is good because it would be unconditionally destroyed when the stack is unwound past it, which could cause stack corruption if somebody was still holding a reference to the object and tried to access it later (You shouldn't be expecting local variables to be valid after their scope ends anyway...).
- You can't use operator delete to destroy the object. This also prevents you from bypassing the reference counted destruction semantics.
Basically, the compiler enforces that you have to use class-defined functions to destroy the object, in this case, Release(). - Anonymous
June 30, 2005
And people wonder why I never, ever use ATL if I can possibly avoid it. I barely understand all the semantics of C++ -- ATL effectively defines a NEW language with subtly different semantics. I don't see how ANYONE gets it right. - Anonymous
June 30, 2005
"And people wonder why I never, ever use ATL if I can possibly avoid it. I barely understand all the semantics of C++ -- ATL effectively defines a NEW language with subtly different semantics. I don't see how ANYONE gets it right."
QTF!! - Anonymous
June 30, 2005
Skywing,
I understand the semantics of a private destructor - the thing I wondered about was the fact that it still allowed inheritance. I thought that a private destructor would additionally prevent inheriting from the class, which apparently it doesn't (or is it a bug in MSVC?)
Regarding ATL: In my lifetime as a COM developer and architect I wrote three COM libraries, which all replaced ATL in parts or entirely. I have seen hundreds of software engineers fail with ATL. COM did make some things easy, but not that easy for most of them.
The last library I wrote even removed the demand to write COM interface maps. The maps were built automatically by inheritance. This library lived without a single macro and was fully debuggable.
Michael - Anonymous
June 30, 2005
The comment has been removed - Anonymous
June 30, 2005
Thursday, June 30, 2005 7:01 PM by Michael Ruck
> I thought that a private destructor would
> additionally prevent inheriting from the
> class, which apparently it doesn't (or is
> it a bug in MSVC?)
What I've seen, and makes sense to me, is that a private destructor prevents some kinds of variables from being declared with that type. Most obvious is that you can't declare a block local variable with that type because of the implicit delete operation when the variable goes out of scope.
In a language that permits "delete this;" in the middle of a method call, it makes sense to allow such variables to be created and deleted on the heap, under their own control.
Of course this doesn't mean that the language really permits it, so it might be a bug in MSVC, I don't know. My stomach isn't strong enough to be a C++ language lawyer. Sometimes I can't even remember how I stomached it for C. - Anonymous
June 30, 2005
I don't get why CFooBase needs to implement IUnknown? Perhaps it's used elsewhere by itself?
ps. I love ATL. Really. - Anonymous
June 30, 2005
I guess there are just a lot of "I HATE ATL" people piping in because of this post. I for one have been using it five years and have hardly had any problems. In this case, we have an instance of someone trying to outsmart ATL with the results of shooting themselves in the foot. - Anonymous
June 30, 2005
I love ATL, and the only reason this code had a problem is because it was trying to do something that it didn't have to do!
The whole CComObject wrapper bit is complex and obfuscating, but I find ATL very good at creating and using COM objects - much much much better, for example, than the horrid compiler 'builtin' functionality (the _bstr_t stuff). - Anonymous
June 30, 2005
Making the destructor private disallows creating instances on the stack (even by subclassing). You have to use some other way, for example:
class nonDestroyableFromOutside
{
private:
~nonDestroyableFromOutside() { };
public:
static nonDestroyableFromOutside * GetNewInstance() { return new nonDestroyableFromOutside; };
static void DestroyThisInstance( nonDestroyableFromOutside * pInst ) { delete pInst; };
};
makes you go through the extra calls to create and destroy an instance. Those calls can then be nicely wrapped in a library (e.g. in smart pointers, like in the ATL). - Anonymous
June 30, 2005
"The good news is that the compiler will complain if there's an ambiguous cast, which forces you to resolve the ambiguity."
The good news is that it will complain if you use static_cast. The bad news is that it will never complain if you use reinterpret_cast, because that will never change the pointer value.
This has bit me in the past, when I wondered why the wrong method is called on my COM-class. And then I realize that I use reinterpret_cast, which happily let me use the vtable for IDispath as if it was the vtable of ISupportErrorInfo. - Anonymous
July 01, 2005
The comment has been removed