AddRef and function signature

Question

I've always used the following rule for signatures of functions that return ref-counted objects based on whether they do an AddRef or not, but want to explain it to my colleagues too... So my question is, is the rule described below a widely followed rule? I'm looking for pointers to (for example) coding rules that advocate this style.

---

If the function does not add a reference to the object, it should be returned as the return value of the function:

class MyClass
{
protected:
    IUnknown *getObj() { return m_obj; }
private:
    IUnknown *m_obj;
};

However, if the function adds a reference to the object, then a pointer-to-pointer of the object is passed as a parameter to the function:

class MyClass
{
public:
    void getObj(IUnknown **outObj) { *outObj = m_obj; (*outObj)->AddRef(); }
private:
    IUnknown *m_obj;
};

Answer 1

It's much more typical to use the reference-counting smart pointers for cases when a new object is created and the caller has to take ownership of it.

Answer 2

I've used this same style on projects with a lot of COM. It was taught to me by a couple of people that learned it when they worked at NuMega on a little thing called SoftICE. I think this is also the style taught in the book "Essential COM", by Don Box (here it is at Amazon). At one point in time this book was considered the Bible for COM. I think the only reason this isn't still the case is that COM has become so much more than just COM.

All that said, I prefer CComPtr and other smart pointers.

Answer 3

One approach is to never use the function's return value. Only use output parameters, as in your second case. This is already a rule anyway in published COM interfaces.

Here's an "official" reference but, as is typical, it doesn't even mention your first case: http://support.microsoft.com/kb/104138

But inside a component, banning return values makes for ugly code. It is much nicer to have composability - i.e. putting functions together conveniently, passing the return value of one function directly as an argument to another.

Smart pointers allow you to do that. They are banned in public COM interfaces but then so are non-HRESULT return values. Consequently, your problem goes away. If you want to use a return value to pass back an interface pointer, do it via a smart pointer. And store members in smart pointers as well.

However, suppose for some reason you didn't want to use smart pointers (you're crazy, by the way!) then I can tell you that your reasoning is correct. Your function is acting as a "property getter", and in your first example it should not AddRef.

So your rule is correct (although there's a bug in your implementation which I'll come to in a second, as you may not have spotted it.)

This function wants an object:

void Foo(IUnknown *obj);

It doesn't need to affect obj's refcount at all, unless it wants to store it in a member variable. It certainly should NOT be the responsibility of Foo to call Release on obj before it returns! Imagine the mess that would create.

Now this function returns an object:

IUnknown *Bar();

And very often we like to compose functions, passing the output of one directly to another:

Foo(Bar());

This would not work if Bar had bumped up the refcount of whatever it returned. Who's going to Release it? So Bar does not call AddRef. This means that it is returning something that it stores and manages, i.e. it's effectively a property getter.

Also if the caller is using a smart pointer, p:

p = Bar();

Any sane smart pointer is going to AddRef when it is assigned an object. If Bar had also AddRef-ed well, we have again leaked one count. This is really just a special case of the same composability problem.

Output parameters (pointer-to-pointer) are different, because they aren't affected by the composability problem in the same way:

Again, smart pointers provide the most common case, using your second example:

myClass.getObj(&p);

The smart pointer isn't going to do any ref-counting here, so getObj has to do it.

Now we come to the bug. Suppose smart pointer p already points to something when you pass it to getObj...

The corrected version is:

void getObj(IUnknown **outObj) 
{
    if (*outObj != 0)
        (*outObj)->Release();

    *outObj = m_obj;
    (*outObj)->AddRef();  // might want to check for 0 here also
}

In practise, people make that mistake so often that I find it simpler to make my smart pointer assert if operator& is called when it already has an object.