ATL bug of CComPtr?

Hello everyone,


In the ATL Internals book, one form of constructor of CComQIPtr is implemented as this,

Code:

CComQIPtr (IUnknown* lp)
{
    p = NULL; if (lp != NULL) lp -> QueryInterface (*piid, (void**)&p);
}

I think there is a bug when QueryInterface fails, and the original value of member variable p is overwritten.

I found the in MSVC 2008, the implementation is,

Code:

atlcomcli.h

	CComQIPtr(_In_opt_ IUnknown* lp) throw()
	{
		if (lp != NULL)
			lp->QueryInterface(*piid, (void **)&p);
	}

Seems the bug is fixed? Is it a bug in old version of ATL or a bug in the book? :-)


thanks in advance,
George

>> Is it a bug in old version of ATL or a bug in the book?
Probably.

gg

Thanks gg,

Originally Posted by Codeplug

>> Is it a bug in old version of ATL or a bug in the book?
Probably.

gg

In my original post, you think the current implementation (MSVC 2008) of CComQIPtr is ok?


regards,
George

Actually, I don't think it's a bug. It's probably an optimization since IUnknown::QueryInterafce() will set it to NULL on failure.

gg

Thanks gg,


Is it possible that internal data member p holds some reference to other instance before we invoke this constructor?

If yes, I think we should call Release on internal data member p, before construction again, in order to release the previous held reference, before gain the reference to the new instance.

Originally Posted by Codeplug

Actually, I don't think it's a bug. It's probably an optimization since IUnknown::QueryInterafce() will set it to NULL on failure.

gg

regards,
George

Originally Posted by George2

Is it possible that internal data member p holds some reference to other instance before we invoke this constructor?

If yes, I think we should call Release on internal data member p, before construction again, in order to release the previous held reference, before gain the reference to the new instance.

I think Codeplug is right. It was never a bug because the code in question is in the constructor. There is no previous value of the member variable. There is nothing to release as the contents are garbage until first set here. You must be thinking of the Attach method.
Removing the first statement will just be an optimisation.

Thanks iMalc,


Question answered.

Originally Posted by iMalc

I think Codeplug is right. It was never a bug because the code in question is in the constructor. There is no previous value of the member variable. There is nothing to release as the contents are garbage until first set here. You must be thinking of the Attach method.
Removing the first statement will just be an optimisation.

regards,
George