A co-worker came by to ask what he thought was a coding "style" question that turned into a correctness issue, and I thought I'd share it.
Someone had defined two COM interfaces:
interface IFoo : IUnknown{ HRESULT FooMethod1(); HRESULT FooMethod2();}
They also had a factory interface IBar which had a method HRESULT GetFoo(IFoo **ppFoo).
As a result of new work, the team that owned IFoo wanted to extend IFoo. To do this, they defined a new interface, IFoo2 that inherited from IFoo:
interface IFoo2 : IFoo{ HRESULT Foo2Method1(); HRESULT Foo2Method2();}
The team that owned IFoo (and IBar) decided that they didn't want to change the IBar interface to add a GetFoo2 method, feeling that the GetFoo method was "good enough".
My co-worker wanted to call GetFoo and cast the resulting IFoo object into an IFoo2 object (he knew that the IFoo he got was always going to be an IFoo2). He was worried about the stylistic implications of doing the cast.
The problem with doing this turns out not to be a style issue, but instead to be correctness issue. Here's the problem.
Somewhere under the covers, there's a class CFoo that implements IFoo and IFoo2. This is the object that will be returned by the IBar::GetFoo method.
When the compiler lays out this object, the compiler will lay out the data for the class as follows:
When IBar::GetFoo returns, it returns a pointer to the 2nd element of the class (the adjustor thunk will ensure that the right thing happens when you call into the member functions).
The IFoo vtable is laid out in memory like this:
The IFoo2 vtable on the other hand is laid out in memory like this:
When the caller calls into a method on IFoo, the compiler will index into the vtable to find the pointer to the code that implements the specified method. By casting from an IFoo to an IFoo2, my co-worker was telling the compiler "I know that this thing is also an IFoo2, so you should act like the vtable is really an IFoo2 vtable.
The first time that he called into Foo2Method1 or Foo2Method2 using this mechanism, if he was lucky, his code would crash. If he was unlucky (and the random chunk of memory sitting at the end of the vtable for IFoo happened to be code that matched the function signature of Foo2Method2, he would simply corrupt memory.
All-in-all, a bad thing to do.
In addition, if the particular class implementation of IFoo chose to implement IFoo2 via COM aggregation (in other words, using the delegator pattern), his cast would defeat that.
The right thing to do in this case is to call QueryInterface on the IFoo object looking for IFoo2, then release the IFoo object since it's no longer needed.
Edit: Fixed typos.
A class that "implements IFoo and IFoo2" is hopefully just inheriting IFoo2, since that necessarily requires implementing IFoo. Every IFoo2 is also an IFoo, and if you have an IFoo2*, then you can cast to IFoo* with impunity, thanks to the magic of single inheritance.
If you have an IFoo*, the only way to make it into an IFoo2* is QueryInterface(), which is, I think, the point of this post.
If there's a style question in this post, it has to do with how to do the cast. I suspect the whole reason the question came up is because the co-worker was using C-style casts, and although (IFoo2*)pFoo is wrong, the compiler will let you do it. If you try to use static_cast<IFoo2*>(pFoo), the compiler will throw an error, exactly because this is wrong.
I suspect that this also has something to do with why CreateObject calls in COM take a parameter specifying the interface of the object that you want to use. In this case, GetFoo should really be something like:
HRESULT GetFoo(IUnknown **ppFoo, IID *piid)
Then, the caller could specify that they wanted an IFoo2 explicitly, and if that wasn't supported, they would get an E_NO_INTERFACE
Actually, in general given a class CFoo : IFoo2, and IFoo2 : IFoo, if you have a IFoo* that you know is actually a CFoo, it's perfectly safe to do static_cast<IFoo2*>(p). The vtable will be adjusted on the cast so it'll work fine. If you're not sure the pointer is an IFoo2 you could use a dynamic_cast which throws an exception if it isn't.
In the context of COM however, you shouldn't assume that's how the interfaces are implemented, and of course you should use QueryInterface.
Speaking of style, I hope nobody still actually prefixes their classes with a C. :P
Based on the code you've provided, I'm afraid that your layout for the class in incorrect.
You gave us the following assumptions:
IFoo : IUnknown and IFoo2 : IFoo
Now you mentioned that CFoo implements both interfaces. However, since IFoo2 inherits from IFoo, that means that it's:
CFoo : IFoo2
Since this is a single-inheritance chain, then adjustor thunks are not necessary, so for a CFoo, IFoo and IFoo2 can (and do) share the same vtable. The one and only vtable is laid out the way that you laid out the IFoo2 vtable. The class will have one vtbl pointer at the top followed by the data for CFoo.
Because of the single inheritance, all casts can and will work just fine. Now I do agree that _depending_ on this is a bad thing, because if IFoo2 were broken out to have no inheritance from IFoo, then the class looks closer to what you've described. Since QueryInterface works no matter what, it is, all in all, the right solution in the long run. But your friend's code was in no danger of crashing.
RyanBemrose:
You're wrong. It's a perfectly valid static_cast to go from IFoo* to IFoo2*. The only time it wouldn't be a valid static_cast would be if there were multiple copies of a base class and you tried to cast to a subclass. For instance, if you had, B:A, C:A, and D:C,B, then you can't cast an A to a D, because there are two copies of A in D.
SvenGroot:
"The vtable will be adjusted on the cast so it'll work fine." - There is no vtable adjustment in this case, since there's only one vtable and the compiler knows it.
Why not change IBar's GetFoo to return an IFoo2? (side note: I hate using foo and bar) - since IFoo2 is an IFoo and the team owns both interfaces, shouldn't that work with little issue?
(Unless other callers are doing similar casting monkey business)
Sven: Actually lots of Windows code prefixes their classes with C - it's a legacy of the old hungarian days.
And the cast isn't safe unless the <i>compiler</i> can tell that it's a CFoo - otherwise it has to assume that it's a class with nothing but a vtable.
Mark: Because that would change the interface contract for IBar, which means that IBar's interface ID would have to change.
QueryInterface isn't a particularly controversial topic (and I haven't heard any grumbling about
"And the cast isn't safe unless the <i>compiler</i> can tell that it's a CFoo - otherwise it has to assume that it's a class with nothing but a vtable."
Right, but given the inheritance tree, that doesn't matter. Your friend's code was never in danger of crashing so long as the class hierarchy matches the assumptions.
I'm sorry, but this whole entry is incorrect. I mean, given this code:
---
class IFoo : public IUnknown
{
public:
virtual HRESULT FooMethod1() = 0;
virtual HRESULT FooMethod2() = 0;
};
class IFoo2 : public IFoo
virtual HRESULT Foo2Method1() = 0;
virtual HRESULT Foo2Method2() = 0;
class CFoo : public IFoo2
HRESULT STDMETHODCALLTYPE QueryInterface(const IID &,void **) { return E_NOTIMPL; }
ULONG STDMETHODCALLTYPE AddRef(void) { return 0; }
ULONG STDMETHODCALLTYPE Release(void) { return 0; }
HRESULT FooMethod1() { return E_NOTIMPL; }
HRESULT FooMethod2() { return E_NOTIMPL; }
HRESULT Foo2Method1() { return E_NOTIMPL; }
HRESULT Foo2Method2() { return E_NOTIMPL; }
private:
int mI;
void main()
CFoo foo;
printf(
"Size: %d\n"
"CFoo *: %x IFoo2 *:%x IFoo *:%x\n",
sizeof( CFoo ),
&foo, static_cast<IFoo2 *>( &foo ), static_cast< IFoo * >(&foo) );
}
You get this output:
Size: 8
CFoo *: 12ff28 IFoo2 *:12ff28 IFoo *:12ff28
It's clear that all casts work, all are based from the same address, and there is only one vtable.
Now, if I would have changed the class decls to:
class IFoo : public virtual IUnknown
class IFoo2 : public virtual IUnknown
class CFoo: public IFoo, public IFoo2
then you get this:
Size: 24
CFoo *: 12ff18 IFoo2 *:12ff20 IFoo *:12ff18
Which is somewhat closer to what you described, where simple casts between IFoo and IFoo2 no longer suffice.
By the way, I'm not trying to be a jerk here. I just don't want people to get the wrong idea about how C++ casts work and how objects are laid out.
Jack, you're not. But what happens if the class derives from IFoo and IFoo2 (I believe that it's legal).
In that case, you have a case of multiple inheritance.
Jack, if you and the compiler both know the structure of the COM object, then why are you even bothering to use COM?
For the current (and past) implementations of vtables in MSVC, what you say is correct. However, what about Borland C++ or any other language that properly supports COM? COM makes no statement as to the structure of the vtables for an object supporting multiple interfaces. It only makes a statement about each interface's vtable. Since we are working with COM, we only have available the rules COM provides us.
Your statements, while correct depend on both the implementation of the object and the compiler in order to prove something true. This is very dangerous in programming. In Larry's case, he is using the very same details (with a easily corrected technical error) to show why something is bad. That is perfectly legit.
BTW
Jack, YOU are giving people the wrong idea about how C++ casts work and how objects are laid out. They are very dangerous things to depend on.
The things that you are saying are compiler implementation details and are NOT addressed in the C++ standard.
"Jack, you're not. But what happens if the class derives from IFoo and IFoo2 (I believe that it's legal)."
Yes, I brought that up in one of my messages.
"Jack, if you and the compiler both know the structure of the COM object, then why are you even bothering to use COM? "
I'm only speaking to the example provided, not to COM in general.
"COM makes no statement as to the structure of the vtables for an object supporting multiple interfaces."
Yes, but this Larry's friend's group does. I'm only speaking to the example, where the people authoring the component set out specific guidelines. Mind you, I don't agree with them either as it violates what COM is designed for, but that doesn't justify confusing people about how vc++ generates a vtable.
In fact, that's really what I'm trying to point out here, that vtables are not generated this way for this single inheritance case in VC++ (or really any compiler).
"Jack, YOU are giving people the wrong idea about how C++ casts work and how objects are laid out. They are very dangerous things to depend on.
Am I?
For one, Larry's post spoke to an implementation defined way of vtable generation, and I corrected it. The source material had nothing to do with the standard, so neither does my rebuttal.
for another, my casting rules are absolutely correct. Given a defined class hierarchy, you can cast from any point to any other point unless more than one copy of the class is in the hierarchy. If you cast to a derived class knowing full well that you ARE that derived class, the cast will give you a valid pointer.
To be fair: As far as I know, given what I stated in the post, Jack is totally right. On Monday, I'm going to talk a bit about how the standard requires that a compiler handle multiple inheritance (it turns out that much of this vtable behavior is actually required by the standard). As I said however, if the implementor of the class had chosen to implement both IFoo and IFoo2, my co-worker would have been up a creek - that's way too fragile to trust.