You're writing some code that uses COM.  Do you use SmartPointers or not use SmartPointers?  Debate on this topic flares up now and then. 

The trick to answering it is to seperate the technical facts from stubbornly held beliefs.  Like any technology, there is a right time and a wrong time to use it.  But that's not what I want to talk about today.  Let's assume you've decided to use SmartPointers.  I posit that CComPtr<> is good and CComQIPtr<> is bad.

The reason for this is simple: CComQIPtr<> masks the return value of the QueryInterface() call. 

Lets take a look at two snippets of code to frame the discussion:

void ExFunc(IFoo *pFoo)
{
    HRESULT hr = pFoo->Init();
    if (SUCCEEDED(hr))
    {
        CComPtr<IBar> spBar;
        hr = pfoo->QueryInterface(IID_PPV_ARGS(spBar));
        if (SUCCEEDED(hr))
        {
            hr = spBar->DoSomething();
        }
    }
}
void ExFunc(IFoo *pFoo)
{
    HRESULT hr = pFoo->Init();
    if (SUCCEEDED(hr))
    {
        hr = E_NOINTERFACE;
        CComQIPtr<IBar> spBar = pFoo;
        if (spBar)
        {
            hr = spBar->DoSomething();
        }
    }
}

Some points:

  1. When you have a large number of people working on the same code base, you want to create patterns in the code that people recognize.  CComQIPtr<> breaks the pattern of COM-call/check-HRESULT.  Instead you have to check for the non-NULLness of the out parameter. 
  2. You can assume the QueryInterface() call failed with E_NOINTERFACE, but you don't know for sure.  What if it failed with some other HRESULT?  Nobody says QueryInterface() can't do things besides just casting this around.  E.g. a function that implements IPersistFile::Load() could delay loading the file until someone does a QueryInterface() for an interface that actually needs the contents of the file it was asked to load.
  3. CComQIPtr<> masks errors in QueryInterface().  The contract for QueryInterface() is the out parameter must be valid if it returns success, otherwise it must be set to NULL.  So, in theory, if (SUCCEEDED(hr)) { ... } and if (spBar) { ... } should be equivalent.  However, people often get QueryInterface() wrong.  It seems to me that it is much more likely an implementer will fail to set the out parameter to NULL on failure than they will fail to return S_OK on success.
  4. Lastly, I just don't like the fact that magic is happening in the assignment.  It is easy to screw things up with COM, and it seems CComQIPtr<> just makes it easier.  CComPtr<> is a lot less magic and more predictable.  It makes it harder to screw things up.

In summary: use CComPtr<> and don't use CComQIPtr<>.  I would especially encourage those who would write Internet Explorer extensions to use CComPtr<>. 

Thoughts?