style: Argument pattern to avoid: "const scoped_refptr<SomeClass>& argument"

[Jim Roskind]

Comments welcome, but unless I hear some really good counter argument, I'm going to continue to question this pattern in code reviews, and I'd suggest other folks do so as well.  I'm hoping that this email will reduce the spread of this hard-to-read pattern.

I saw some virally replicating code that seemed to use a rather strange argument passing pattern that I'll "just" call "const-scoped_refptr-ref":

void SomeFunctionOrMethod(const scoped_reftrp<SomeClass>& argument);

If you read that code, would you think |argument| is in an input, or an output?

The style guide suggests using either pointers (if we need a mutable instance for returning data), or a const ref (if we want the parameter to be a read-only input).  The above code sort-of abides by that rule, but sadly loses the desired in-vs.-out distinction, and adds a ton of text.  I believe that the above function would be better written MUCH more simply as EITHER:

void SomeFunctionOrMethod(SomeClass* argument);

void SomeFunctionOrMethod(const SomeClass& argument);

(Choose one flavor, depending on how you intend to use the argument).  It clearly tells the reader what the intent was; it runs about as fast (maybe faster); and it is much more readable.

WHY?

For all the above prose in the the "const-scoped_refptr-ref," it turns out that |argument| is still mutable! Despite the "const" there is no impact on the argument.  Hence you can't tell whether it was an input arg our an output arg.  It is also much harder to read or even see the real type of the arg <sigh>.  I'd also bet that an implementation would generally do *2* dereferences each time the argument was used.  One indirection to find the caller's scoped_refptr, and one to find the current underlying instance.  :-(

I suspect some folks figured the "const-scoped_refptr-ref" might be a useful pattern because it avoided an IncRef() that might have happened if we passed in a full blown "scoped_refptr."  After all, the "style rules" say don't generally use copy-constructors on arguments, and consider the use of a "const ref" for a class ((?)like a scoped_refptr :-/ ).  In this case, the "class" is really just a pointer.  We're better off using the underlying raw pointer, assuming we don't need the IncRef() for lifetime safety (and "const-scoped_refptr-ref" doesn't give us an IncRef()).  If you need the lifetime safety, because some recursion etc. might release the reference held in the calling site, then passing a real scoped_refptr would make sense.

Comments and arguments/flames welcome.

Jim

p.s., ...and I really hope none of the uses assumed that the caller would change the underlying scoped_refptr *while* the called function was running!?!  A compiler would probably have to support that, leading to the double de-references on each access, but the resulting code would be very surprising to me.

[Mark Mentovai]

Jim Roskind wrote:
> Comments welcome

You’re correct and I agree.

> I suspect some folks figured the "const-scoped_refptr-ref" might be a useful
> pattern

This is pessimistic, but I bet that the answer is far simpler. I think
this happens most frequently when someone needs to write a function to
operate on some object that’s held in existing code as a
scoped_refptr<SomeClass>. I doubt anyone writing |const
scoped_refptr<SomeClass>&| is actually thinking about reference
counting, they’re just thinking about ferrying an object over to
another function and operating by rote.

Mark

[William Chan (陈智昌)]

As I've been using const scoped_refptr<T>& (and am probably mostly responsible for the appearance of this idiom in chromium code), I will attempt to address this.  Thanks for bringing this up on chromium-dev, I probably should have done so before introducing an inconsistent idiom.  My bad.

I think you raise some very good points.  Since you haven't elucidated many counterpoints, I shall attempt to do so.

* implicit T* and scoped_refptr<T> conversion leads to bugs (see: http://code.google.com/p/chromium/issues/detail?id=28083).  If we agree this bug-prone pattern is worth stopping, then we shouldn't use a pattern that encourages implicit conversion.

* const shared_ptr<T>& is a very common, especially within google3 code (googlers can search for this).  I can't come up with a good public codesearch link as most of the results are boost/tr1 library implementations instead.  As scoped_refptr is fairly similar, then I don't think it's too unreasonable to say that const SmartPtr<T>& is not an uncommon pattern.

* const scoped_refptr<T>& is self-documenting that T is a refcounted object.  T* offers no such assistance here.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Peter Kasting]

On Wed, Sep 1, 2010 at 3:54 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

* implicit T* and scoped_refptr<T> conversion leads to bugs (see: http://code.google.com/p/chromium/issues/detail?id=28083).  If we agree this bug-prone pattern is worth stopping, then we shouldn't use a pattern that encourages implicit conversion.

I don't think I completely follow your argument.  I happen to agree that implicit conversion between these two things is a potential bug source.  That doesn't suggest to me that I should never convert them; it suggests that I should need to do so explicitly, i.e. call functions like "foo(my_ptr.get())".

* const shared_ptr<T>& is a very common, especially within google3 code (googlers can search for this).  I can't come up with a good public codesearch link as most of the results are boost/tr1 library implementations instead.  As scoped_refptr is fairly similar, then I don't think it's too unreasonable to say that const SmartPtr<T>& is not an uncommon pattern.

I have no google3 experience.  I imagine I'm not unique on the team.  I'm definitely not unique in the general public that can read and contribute to the codebase.  I would not use google3 as a reference for any discussion about Chromium code.

 

* const scoped_refptr<T>& is self-documenting that T is a refcounted object.  T* offers no such assistance here.

That doesn't matter unless we're actually wanting to modify the refcount, take ownership of the object, etc.  When you only need access to an object, and nothing to do with ownership, you should use raw pointers.

When you _do_ need to muck with ownership, passing a "scoped_refptr" in any form is weird and confusing, since the scope is changing.

Therefore I agree with Jim and Mark that we should never use this idiom.

PK 

[William Chan (陈智昌)]

On Wed, Sep 1, 2010 at 4:01 PM, Peter Kasting <pkas...@google.com> wrote:

On Wed, Sep 1, 2010 at 3:54 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

* implicit T* and scoped_refptr<T> conversion leads to bugs (see: http://code.google.com/p/chromium/issues/detail?id=28083).  If we agree this bug-prone pattern is worth stopping, then we shouldn't use a pattern that encourages implicit conversion.

I don't think I completely follow your argument.  I happen to agree that implicit conversion between these two things is a potential bug source.  That doesn't suggest to me that I should never convert them; it suggests that I should need to do so explicitly, i.e. call functions like "foo(my_ptr.get())".

Hm, I think you're trading one ugliness for another.  Instead of the consistency of using const scoped_refptr<T>&, then you get a lot of .get() and make_scoped_refptr() calls.  I will agree that my argument doesn't immediately follow through from the bugginess of implicit conversion.

 

* const shared_ptr<T>& is a very common, especially within google3 code (googlers can search for this).  I can't come up with a good public codesearch link as most of the results are boost/tr1 library implementations instead.  As scoped_refptr is fairly similar, then I don't think it's too unreasonable to say that const SmartPtr<T>& is not an uncommon pattern.

I have no google3 experience.  I imagine I'm not unique on the team.  I'm definitely not unique in the general public that can read and contribute to the codebase.  I would not use google3 as a reference for any discussion about Chromium code.

Fair enough.  Here's a public codesearch link http://www.google.com/codesearch?hl=en&start=80&sa=N&filter=00&q=const+shared_ptr%3C.*%3E%26.  Apparently I just had to skip a number of pages to get past boost/tr1 libraries.

 

 

* const scoped_refptr<T>& is self-documenting that T is a refcounted object.  T* offers no such assistance here.

That doesn't matter unless we're actually wanting to modify the refcount, take ownership of the object, etc.  When you only need access to an object, and nothing to do with ownership, you should use raw pointers.

I weakly agree to the latter point.  But oftentimes, in a method, you don't necessarily know if the refcount will be modified.  It's only towards the leaf of the call hierarchy that you do.

 

When you _do_ need to muck with ownership, passing a "scoped_refptr" in any form is weird and confusing, since the scope is changing.

I'm not sure I buy this point.  Does this mean you'd be in favor of dropping the copy constructor for scoped_refptr, to force users to use scoped_refptr::get() instead to make it less weird and confusing?

[Peter Kasting]

On Wed, Sep 1, 2010 at 4:16 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Sep 1, 2010 at 4:01 PM, Peter Kasting <pkas...@google.com> wrote:

it suggests that I should need to do so explicitly, i.e. call functions like "foo(my_ptr.get())".

Hm, I think you're trading one ugliness for another.  Instead of the consistency of using const scoped_refptr<T>&, then you get a lot of .get() and make_scoped_refptr() calls.

You should get some get() calls.  I don't know about getting a lot of make_scoped_refptr() calls, I'm not sure why that would be the case in single-threaded code that isn't changing the refcount.

 

Fair enough.  Here's a public codesearch link http://www.google.com/codesearch?hl=en&start=80&sa=N&filter=00&q=const+shared_ptr%3C.*%3E%26.  Apparently I just had to skip a number of pages to get past boost/tr1 libraries.

OK.  That still doesn't make this a "common idiom" in the Chromium codebase.  (And it certainly isn't a common idiom in my career, where I've never really seen this pattern.)  Plus, I would be willing to hazard a guess that I wouldn't like most of those uses either :)

When you _do_ need to muck with ownership, passing a "scoped_refptr" in any form is weird and confusing, since the scope is changing.

I'm not sure I buy this point.  Does this mean you'd be in favor of dropping the copy constructor for scoped_refptr, to force users to use scoped_refptr::get() instead to make it less weird and confusing?

Yes, I would be in favor of dropping the copy constructor.  I think copying a scoped_refptr to another scoped_refptr may be a sign that something is wrong, and where it isn't, a different and more explicit syntax would be clearer.

PK

[Mark Mentovai]

William Chan (陈智昌) wrote:
> * const scoped_refptr<T>& is self-documenting that T is a refcounted object.
>  T* offers no such assistance here.

You mean beyond the fact that T derives from base::RefCounted? That’s
huge. That’s everything.