Passing scoped_refptr

[Václav Brožek]

Hi Chrome developers,

In some recent CLs an interesting question about passing scoped_refptr came up:

Suppose A is a refcounted class. Passing a pointer to A can happen in 3 ways:

(1) void f(scoped_refptr<A> ptr);

(2) void f(A* ptr);

(3) void f(const scoped_refptr<A>& ptr);

All of them happen in our codebase. Some properties:

(1) is the safest, it guarantees that A is not destroyed even if the scoped_refptr passed in as an argument is accessed on different threads. However, to pay for that, (1) induces one more increase and decrease of the refcount.

(2) is less safe, but reasonable as long as it is safe to assume that if *ptr was alive when calling f, it will stay alive until f is finished. In particular, using *ptr only on one thread guarantees that. For that, (2) does not induce any additional refcount changes.

(3) is essentially (2) + requiring that the passed argument was in fact a scoped_refptr.

The questions

Is there any guidance to what to prefer? https://www.chromium.org/developers/smart-pointer-guidelines explicitly mentions (3) as allowed, although it does not explain why not just use (2) instead.

Would a reasonable guidance be to use (2), except in cases where the threading might make it dangerous, in which case (1) is suggested?

Thanks,

Vaclav

[Anand]

It looks like the benefit of (3) is that it ensures that the caller (or the caller's caller, or so on) has a reference to the object via a scoped_refptr for the life of the function call (I know this isn't 100% accurate). You can't say that about (2).

[Sylvain Defresne]

When dealing with pointers and smart pointers, I'm always following this rule of thumb: use raw pointers for non-owning pointers and smart pointer only when you want to transfer/share ownership.

So, I would use (2) if "f" is only using ptr, but does not care about ownership (in particular "f" implementation must not create a scoped_refptr from ptr).

I see (1) and (3) as equivalent as meaning that you want to share ownership of ptr with "f". (1) is safer, but if "f" do copy ptr to another scoped_refptr (like is usually done in a constructor), then (3) is safe too.

TL;DR: I think that (3) should be limited to constructors or setters, while the other constructions should be used for all other functions/methods depending on whether we want to transfer/share ownership (1) or not (2).

-- Sylvain

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

[Ilya Kulshin]

My preference would be to use (2). Regarding ownership, f shouldn't care whether the caller is holding the pointer in a smart pointer, and the signature should not expose the internals of what f plans to do with the pointer. The advantage of refptrs is ownership can be shared, so f can take ownership if it needs to, or not otherwise.

[Bernhard Bauer]

+Dana Jansens 

I would use them in the following ways:

• (1) if f() always takes a reference to the object. Note that if the caller gives up their reference by using std::move, there is no refcount decrease/increase necessary.
  
• (2) if f() never takes a reference to the object. In this case it's purely the caller's responsibility to make sure that the object is alive throughout the method, and the f() doesn't get to touch the refcount at all.
  
• (3) if f() might take a reference to the object. If it doesn't, the scoped_refptr is never copied, so there is not unnecessary refcount churn either.
  

Bernhard.

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Gabriel Charette]

Agreed with using (2) when f() doesn't care about ownership (or even const* if it only cares about const access)

(3) used to be better than (1) when f() took a reference as it avoided an extra reference for the temporary argument.

Now that we have move semantics though I guess (1) is always preferable to (3) as it allows a zero reference bump flow when the caller is giving up ownership via std::move (and f() is also taking it via std::move as well).
With (3) there will always be at least one reference bump when handing ownership as std::move isn't allowed on const objects.

(Note: I'm not an expert on move semantics, please let me know if I missed something there)

Gab

[Daniel Cheng]

We should definitely *not* use raw pointers (2) universally: for uniquely owned objects, we prefer to pass by scoped_ptr<T> rather than a T* with a comment "this raw pointer owns the object". Abstracting away whether or not f() takes ownership doesn't buy much and can definitely make understanding what has ownership harder, e.g. this function takes a T*, do I need to hold on to a refcount on its behalf or not?

I used to encourage passing by const ref (3) if f() takes ownership, and I know several other Chromium reviewers did as well. Despite this, most developers wrote code that passed scoped_refptr by value (1): when I checked (this was before scoped_refptr was movable), it was something like 2:1 in favor of pass by value.

In the end, I'm not sure the extra ref/deref mattered all that much except conceptually… and now that scoped_refptr is movable, the distinction between (1) and (3) matters even less.

Daniel

[David Benjamin]

Purely aesthetically and not at all scientifically, I think const scoped_refptr<T>& looks ridiculous and is kind of a mouthful. :-) If the callee is supposed to take a reference, now that we have move semantics, I agree we should do (1).

If the callee is not supposed to take a reference, I'm not sure. Maybe we still need (3), much as I dislike it. Or maybe we shouldn't care enough and just stick with (1) everywhere? By analogy to scoped_ptr, we'd do (2) for non-owning and (1) for owning. But a function passed a non-owning T* can't suddenly make a scoped_ptr<T> of it without exploding. scoped_refptr, on the other hand, lets one freely turn T* into scoped_refptr<T>, and owning raw pointers makes everything complicated.

If we move to shared_ptr and forbid (or at least discourage) std::enable_shared_from_this, I believe this is no longer possible? In that case, I expect (1) if you're allowed to take a reference and (2) if you aren't (and we'd actually have a vague assurance of this) would be best.

David

[Gabriel Charette]

Agreed that we should use scoped_ptr over scoped_refptr as much as possible, but I think this discussion is about the style recommandation for scoped_refptr which with move semantics can now actually be the same as scoped_ptr I think.

Do we agree that for both scoped_ptr and scoped_refptr we should do (1) when f() is allowed to take ownership and (2) when it isn't (and definitely *never* do (2) with a "takes ownership" comment: putting a raw pointer in a managed pointer is always wrong except on allocation IMO).

scoped_ptr has been used in this way for a while, to me scoped_ptr (1) versus raw pointer (2) is an implicit contract easily assessed from the method signature and scoped_refptr should use the same rules now that they don't incur extra ref/deref.

Re: "the extra ref/deref doesn't matter all that much": maybe, not sure for RefCountedThreadSafe, either way if we recommend the same as scoped_ptr for scoped_refptr we get all the advantages and simpler style rules (win?)

[Peter Kasting]

On Thu, Feb 18, 2016 at 11:49 AM, Gabriel Charette <g...@chromium.org> wrote:

Agreed that we should use scoped_ptr over scoped_refptr as much as possible, but I think this discussion is about the style recommandation for scoped_refptr which with move semantics can now actually be the same as scoped_ptr I think.

Do we agree that for both scoped_ptr and scoped_refptr we should do (1) when f() is allowed to take ownership and (2) when it isn't (and definitely *never* do (2) with a "takes ownership" comment: putting a raw pointer in a managed pointer is always wrong except on allocation IMO).

We should always do (1) when a function is to take ownership.  But I'm not sure we should command (2) in other cases.  I actually sort of like that (3) explicitly says "someone else owns me".  There's no ambiguity, as there can be with a raw pointer.

PK

[Antoine Labour]

(3) would pretty much never be appropriate for scoped_ptr, so I'm not convinced it should be the default for scoped_refptr. It adds an extra indirection, which means more code size and ever-so-slightly more pressure on d-cache. I understand that locally it'll make no measurable difference in most cases, but if we're talking about a style-guide-like recommendation, asymptotically all the code would converge to that, and it'd matter globally.

I think this discussion is based around the idea that the ownership pattern (single vs shared) is a property of the type rather than the instance and/or the algorithm - which is only true today because of RefCounted. But that changes with shared_ptr. If an algorithm operates on a T* without any relation wrt ownership, there is no reason why scoped_refptr should be involved (for the same reason that scoped_ptr wouldn't be).

Antoine

PK

[Peter Kasting]

On Thu, Feb 18, 2016 at 12:40 PM, Antoine Labour <pi...@google.com> wrote:

On Thu, Feb 18, 2016 at 11:58 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 11:49 AM, Gabriel Charette <g...@chromium.org> wrote:

Agreed that we should use scoped_ptr over scoped_refptr as much as possible, but I think this discussion is about the style recommandation for scoped_refptr which with move semantics can now actually be the same as scoped_ptr I think.

Do we agree that for both scoped_ptr and scoped_refptr we should do (1) when f() is allowed to take ownership and (2) when it isn't (and definitely *never* do (2) with a "takes ownership" comment: putting a raw pointer in a managed pointer is always wrong except on allocation IMO).

We should always do (1) when a function is to take ownership.  But I'm not sure we should command (2) in other cases.  I actually sort of like that (3) explicitly says "someone else owns me".  There's no ambiguity, as there can be with a raw pointer.

(3) would pretty much never be appropriate for scoped_ptr,

Why?

It seems like the pros and cons for scoped_ptr and shared_ptr here are similar.

PK

[Gabriel Charette]

In which situations would f() not want to take ownership but care that "someone else has ownership". When getting a raw pointer in f() I sure hope it's assumed that somebody else owns it (so I guess the question is more about knowing that the caller itself is an owner? And I can't think of a situation where that's relevant).

PK

[Antoine Labour]

If all you have in an object is a T* because you don't take ownership, it is invalid to put that T* into a scoped_ptr<T>, so it is not appropriate for a function to require a const scoped_ptr<T>&.

Antoine

 

PK

[Peter Kasting]

Not all functions which take T*s are signalling, by taking a T*, that they do not take ownership.  We have a large number of functions in our codebase today which take ownership of raw pointers passed to them.

Whereas, if something is passed in as const scoped_ptr&, it is extremely clear that the function _must not_ take ownership.

I'd love if ownership was only ever held by scoping objects and no functions could ever take ownership except of something they create locally or receive as a scoped_ptr, but that's not even close to true today.  In the meantime, I don't think it's harmful to allow passing scoped_ptrs by const ref.  I wouldn't require it, but I'm opposed to banning it.

PK

[Antoine Labour]

On Thu, Feb 18, 2016 at 1:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 12:55 PM, Antoine Labour <pi...@google.com> wrote:

On Thu, Feb 18, 2016 at 12:44 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 12:40 PM, Antoine Labour <pi...@google.com> wrote:

On Thu, Feb 18, 2016 at 11:58 AM, Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 11:49 AM, Gabriel Charette <g...@chromium.org> wrote:

Agreed that we should use scoped_ptr over scoped_refptr as much as possible, but I think this discussion is about the style recommandation for scoped_refptr which with move semantics can now actually be the same as scoped_ptr I think.

Do we agree that for both scoped_ptr and scoped_refptr we should do (1) when f() is allowed to take ownership and (2) when it isn't (and definitely *never* do (2) with a "takes ownership" comment: putting a raw pointer in a managed pointer is always wrong except on allocation IMO).

We should always do (1) when a function is to take ownership.  But I'm not sure we should command (2) in other cases.  I actually sort of like that (3) explicitly says "someone else owns me".  There's no ambiguity, as there can be with a raw pointer.

(3) would pretty much never be appropriate for scoped_ptr,

Why?

It seems like the pros and cons for scoped_ptr and shared_ptr here are similar.

If all you have in an object is a T* because you don't take ownership, it is invalid to put that T* into a scoped_ptr<T>, so it is not appropriate for a function to require a const scoped_ptr<T>&.

Not all functions which take T*s are signalling, by taking a T*, that they do not take ownership.  We have a large number of functions in our codebase today which take ownership of raw pointers passed to them.

Whereas, if something is passed in as const scoped_ptr&, it is extremely clear that the function _must not_ take ownership.

Sure, but the function is not usable in any case where the caller doesn't have ownership, or if the object is not heap-allocated. It's an anti-pattern... It's (fortunately) virtually non-existent in the codebase. I counted 42 such functions, where 5 of them are conceptually wrong (they use a const scoped_ptr& to receive a base::Passed value via base::Bind, so the intent is actually to indicate that they *do* take ownership). About half of the remaining ones are helper functions (typically in tests) that are called repeatedly on actual scoped_ptr fields, so they use const scoped_ptr& to save a .get() at the callsite (a legitimate use in my book). In one case, the const scoped_ptr& forced the caller to allocate the object on the heap instead of the stack.

I'd love if ownership was only ever held by scoping objects and no functions could ever take ownership except of something they create locally or receive as a scoped_ptr, but that's not even close to true today.  In the meantime, I don't think it's harmful to allow passing scoped_ptrs by const ref.  I wouldn't require it, but I'm opposed to banning it.

I think we should aim towards fixing functions that take ownership of a T*, rather than introducing an inefficient and wordy workaround that limits reuse.

Antoine

PK

[Peter Kasting]

On Thu, Feb 18, 2016 at 2:59 PM, Antoine Labour <pi...@google.com> wrote:

On Thu, Feb 18, 2016 at 1:38 PM, Peter Kasting <pkas...@chromium.org> wrote:

Not all functions which take T*s are signalling, by taking a T*, that they do not take ownership.  We have a large number of functions in our codebase today which take ownership of raw pointers passed to them.

Whereas, if something is passed in as const scoped_ptr&, it is extremely clear that the function _must not_ take ownership.

Sure, but the function is not usable in any case where the caller doesn't have ownership, or if the object is not heap-allocated. It's an anti-pattern... It's (fortunately) virtually non-existent in the codebase. I counted 42 such functions, where 5 of them are conceptually wrong (they use a const scoped_ptr& to receive a base::Passed value via base::Bind, so the intent is actually to indicate that they *do* take ownership). About half of the remaining ones are helper functions (typically in tests) that are called repeatedly on actual scoped_ptr fields, so they use const scoped_ptr& to save a .get() at the callsite (a legitimate use in my book). In one case, the const scoped_ptr& forced the caller to allocate the object on the heap instead of the stack.

I know imposing is rude, but since you just looked at these, would you be willing to patch them to remove usage of this idiom entirely?  Even with the ones where you said "this is OK", if we're going to say "don't do this", we should just bite the bullet and not do it.  Adding .get()s at the callsites would be a minor price to pay for global consistency.

Although I guess since we're also talking about scoped_refptr here, maybe "global consistency" would mean fixing all such cases for _either_ object.

I'd love if ownership was only ever held by scoping objects and no functions could ever take ownership except of something they create locally or receive as a scoped_ptr, but that's not even close to true today.  In the meantime, I don't think it's harmful to allow passing scoped_ptrs by const ref.  I wouldn't require it, but I'm opposed to banning it.

I think we should aim towards fixing functions that take ownership of a T*, rather than introducing an inefficient and wordy workaround that limits reuse.

I want us to fix passing raw pointers to functions taking ownership too, and try to allow no new ones + ask for existing ones to be cleaned up in my code reviews.  I was not trying to propose that we stop or slow down that process.

My conception of these was that they could be used within a narrow scope where callees understood the needs of their callers (a la the "save a .get()" cases you found) and knew they weren't imposing a hardship, e.g. forcing unnecessary heap-allocation.  I don't think it would be a good idea to pass const scoped_refptr& for public-facing APIs meant to be used at many spots around the codebase.  Since I didn't make that clear before, perhaps that changes your reaction.

Still, if there's any confusion at all about when the use of these is appropriate -- and since I don't think you automatically assumed I meant the restrictions I did, I believe there _is_ such confusion -- it might be better to make the hard rule to ban these and enforce it with an automated check.  Hence the suggestion above.

PK

[Antoine Labour]

I'd be happy to contribute fixing those const scoped_ptr& cases if that allows for a stricter rule ("don't use const scoped_ptr& to indicate the function is not taking ownership"). Keep in mind that you can't eliminate all functions that take a const scoped_ptr&, for example functors passed to std::algorithm functions, so I don't think there's a good way to make an automated check.

I'm not signing up for removing const scoped_refptr& cases. There's ~4500 of them, and each of them requires careful understanding of the function to know whether it takes ownership (-> scoped_refptr<T> and fix callers to use std::move as appropriate) or not (-> (const) T*), let alone cases where it isn't statically decidable (in which case passing scoped_refptr<T> may cause perf regressions). I unfortunately can only spend very little time writing chrome code. Similarly, I think there's a good way to make an automated check for the same reason, although maybe the end goal could be to make scoped_refptr move-only (with an explicit scoped_refptr MakeNewRef()). We should probably get consensus on where to go first (in particular wrt shared_ptr).

Antoine

 

PK

[Peter Kasting]

On Thu, Feb 18, 2016 at 3:40 PM, Antoine Labour <pi...@google.com> wrote:

I'm not signing up for removing const scoped_refptr& cases. There's ~4500 of them, and each of them requires careful understanding of the function to know whether it takes ownership (-> scoped_refptr<T> and fix callers to use std::move as appropriate) or not (-> (const) T*), let alone cases where it isn't statically decidable (in which case passing scoped_refptr<T> may cause perf regressions).

If there are this many cases, then perhaps it's not true that, as posited earlier, scoped_refptr<T> is like scoped_ptr<T> here and we should have the same rules for both.

PK

[Sunny Sachanandani]

Just to recap the various differences between scoped_ptr and scoped_refptr:

* Ownership semantics for base::RefCounted is a property of the type but for scoped_ptr semantics is a property of the pointer, e.g. you could release the object from a scoped_ptr and put it into an std::shared_ptr if you wanted.

* For scoped_ptr we use std::move to transfer ownership but with base::RefCounted we either copy the scoped_refptr or create a new scoped_refptr for sharing ownership.

* For scoped_refptr we can use const scoped_refptr<T>& (callee creates copy) or std::move (caller creates copy) to avoid extraneous ref counting but for scoped_ptr using const scoped_ptr<T>& is a means to avoid calling get() which should be inlined anyway.

With that being said, I prefer:

* Callee takes T* for indicating that ownership is neither being transferred nor shared. Caller guarantees that object is alive during the entirety of the call.

* Callee takes scoped_ptr<T> for indicating that ownership is being transferred. Caller calls std::move here because scoped_ptr is a move-only type.

* Callee takes scoped_refptr<T> for indicating that ownership is being shared. Caller calls std::move if ownership is being transferred and not if ownership is being shared. This also avoids extraneous ref counting.

e.g. X::Create(scoped_refptr<Y> y) transfers ownership of y to X's constructor so it calls X(std::move(y)). But X::Create's caller usually only wants to share ownership so it calls X::Create(y).

This scheme also has the advantage of working with std::shared_ptr if we ever migrate to using that.

--

[Antoine Labour]

On Thu, Feb 18, 2016 at 4:18 PM, Sunny Sachanandani <sun...@chromium.org> wrote:

Just to recap the various differences between scoped_ptr and scoped_refptr:

* Ownership semantics for base::RefCounted is a property of the type but for scoped_ptr semantics is a property of the pointer, e.g. you could release the object from a scoped_ptr and put it into an std::shared_ptr if you wanted.

* For scoped_ptr we use std::move to transfer ownership but with base::RefCounted we either copy the scoped_refptr or create a new scoped_refptr for sharing ownership.

* For scoped_refptr we can use const scoped_refptr<T>& (callee creates copy) or std::move (caller creates copy) to avoid extraneous ref counting but for scoped_ptr using const scoped_ptr<T>& is a means to avoid calling get() which should be inlined anyway.

With that being said, I prefer:

* Callee takes T* for indicating that ownership is neither being transferred nor shared. Caller guarantees that object is alive during the entirety of the call.

* Callee takes scoped_ptr<T> for indicating that ownership is being transferred. Caller calls std::move here because scoped_ptr is a move-only type.

* Callee takes scoped_refptr<T> for indicating that ownership is being shared. Caller calls std::move if ownership is being transferred and not if ownership is being shared. This also avoids extraneous ref counting.

e.g. X::Create(scoped_refptr<Y> y) transfers ownership of y to X's constructor so it calls X(std::move(y)). But X::Create's caller usually only wants to share ownership so it calls X::Create(y).

This scheme also has the advantage of working with std::shared_ptr if we ever migrate to using that.

+1

Antoine

[Peter Kasting]

On Thu, Feb 18, 2016 at 4:18 PM, Sunny Sachanandani <sun...@chromium.org> wrote:

With that being said, I prefer:

* Callee takes T* for indicating that ownership is neither being transferred nor shared. Caller guarantees that object is alive during the entirety of the call.

* Callee takes scoped_ptr<T> for indicating that ownership is being transferred. Caller calls std::move here because scoped_ptr is a move-only type.

* Callee takes scoped_refptr<T> for indicating that ownership is being shared. Caller calls std::move if ownership is being transferred and not if ownership is being shared. This also avoids extraneous ref counting.

e.g. X::Create(scoped_refptr<Y> y) transfers ownership of y to X's constructor so it calls X(std::move(y)). But X::Create's caller usually only wants to share ownership so it calls X::Create(y).

Here's my attempt at restating this in an imperative form for use in one of our guides.

***

Object ownership and calling conventions

When functions need to take raw or smart pointers as parameters, use the following conventions.  Here we refer to the parameter type as T and name as |t|.

• If the function does not modify |t|'s ownership, declare the param as T*.  The caller is expected to ensure |t| stays alive as long as necessary, generally through the duration of the call.  Exception: In rare cases (e.g. using lambdas with STL algorithms over containers of scoped_ptrs), you may be forced to declare the param as const scoped_ptr<T>&.  Do this only when required.
  
• If the function takes ownership of a non-refcounted object, declare the param as scoped_ptr<T>.
• If the function takes a ref on a refcounted object, declare the param as scoped_refptr<T>.  The caller can decide whether it wishes to transfer ownership (by calling std::move(t) when passing |t|) or retain its ref (by simply passing |t| directly).

In short, functions should never take ownership of parameters passed as raw pointers, and there should rarely be a need to pass smart pointers by const ref.

***

Does this look good?  Are there any reasons to make any exceptions for const scoped_refptr&?

PK

[David Benjamin]

I like this, at least as an end state. I don't have strong views on whether we go straight here, or first account further for the non-owning T* -> scoped_refptr<T> problem. It sounds like people are leaning towards going straight here, which SGTM.

In that case, what do folks think about making the scoped_refptr<T>(T*) constructor explicit, to match scoped_ptr<T>(T*)? I'm sure that will break everything right now and we'll probably need a clang tool somewhere, but it aligns better with the proposed rules.

David

[Sunny Sachanandani]

On Thu, Feb 18, 2016 at 4:48 PM, David Benjamin <davi...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 7:42 PM Peter Kasting <pkas...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 4:18 PM, Sunny Sachanandani <sun...@chromium.org> wrote:

With that being said, I prefer:

* Callee takes T* for indicating that ownership is neither being transferred nor shared. Caller guarantees that object is alive during the entirety of the call.

* Callee takes scoped_ptr<T> for indicating that ownership is being transferred. Caller calls std::move here because scoped_ptr is a move-only type.

* Callee takes scoped_refptr<T> for indicating that ownership is being shared. Caller calls std::move if ownership is being transferred and not if ownership is being shared. This also avoids extraneous ref counting.

e.g. X::Create(scoped_refptr<Y> y) transfers ownership of y to X's constructor so it calls X(std::move(y)). But X::Create's caller usually only wants to share ownership so it calls X::Create(y).

Here's my attempt at restating this in an imperative form for use in one of our guides.

***

Object ownership and calling conventions

When functions need to take raw or smart pointers as parameters, use the following conventions.  Here we refer to the parameter type as T and name as |t|.

• If the function does not modify |t|'s ownership, declare the param as T*.  The caller is expected to ensure |t| stays alive as long as necessary, generally through the duration of the call.  Exception: In rare cases (e.g. using lambdas with STL algorithms over containers of scoped_ptrs), you may be forced to declare the param as const scoped_ptr<T>&.  Do this only when required.
  
• If the function takes ownership of a non-refcounted object, declare the param as scoped_ptr<T>.
• If the function takes a ref on a refcounted object, declare the param as scoped_refptr<T>.  The caller can decide whether it wishes to transfer ownership (by calling std::move(t) when passing |t|) or retain its ref (by simply passing |t| directly).

In short, functions should never take ownership of parameters passed as raw pointers, and there should rarely be a need to pass smart pointers by const ref.

***

Does this look good?  Are there any reasons to make any exceptions for const scoped_refptr&?

I think std::move could be used in place of const scoped_refptr<T>& - I find the latter way too wordy anyway. Maybe this should be mentioned in the guide.

We should also decide if these rules apply to return values of functions especially getters.

[Antoine Labour]

Yes to me.

 

Are there any reasons to make any exceptions for const scoped_refptr&?

I think the same as for scoped_ptr. If you want to, say, sort your std::vector<scoped_refptr<T>> you need to provide a comparator that takes const scoped_refptr<T>&. Taking scoped_refptr<T> would compile, but the refcount trashing would likely be disastrous perf-wise.

Antoine
 

PK

[Antoine Labour]

I support working towards that.

Antoine

 

David

[Daniel Cheng]

+1, we should do this

 

Antoine

 

David

[Sylvain Defresne]

+1, I like that!

-- Sylvain

[Václav Brožek]

Thanks for all the interesting and insightful responses.

I'm also in favour of Peter's proposal.

We should make sure that if we introduce this guideline, then it is aligned with https://www.chromium.org/developers/smart-pointer-guidelines (currently it whitelists const scoped_refptr& as a parameter to avoid extra refcount churn).

Thanks!

Vaclav

[Gabriel Charette]

+1, I like it. 

The last bullet perhaps needs to be more contrastful : s/takes a ref/may take a ref/ ?

Should the last bullet also explicitly state that "the function should call std::move(t) to take ownership". Otherwise people could respect the rule but still incur an unnecessary ref bump via the temporary argument if its ref is copied instead of taken.

Thanks for putting this together,

Gab

[Daniel Murphy]

+Kinuko Yasuda, did you have any more reasons for preferring "const scoped_refptr<T>&"? I think I remembering you asking for these changes in a couple of my patches. Sort performance above makes sense.

[Gabriel Charette]

+dm...@chromium.org +ki...@chromium.org

I think the only reason for the old rule stating to prefer "const scoped_refptr<T>&" was that it avoided a ref count bump for the temporary argument. This still resulted in an unnecessary refcount +/- if the caller was giving up ownership though but we couldn't avoid that at the time.

With C++11 and move-semantics, we can avoid all unnecessary refcount bumps with "scoped_refptr<T>" as the caller can give (std::move) its reference away and the recipient can take it (std::move again) from the temporary argument.

[Vladimir Levin]

Hi,

I support all of this :P However, I had a question: I know we have a pattern in some spots like this:

class Foo {

 public:

  Bar* get_bar() { return bar_.get(); }

 private:

  scoped_refptr<Bar> bar_;

};

And sometimes the code does this:

foo.get_bar()->DoSomething(); // don't need ownership

scoped_refptr<Bar> baz(foo.get_bar()); // share ownership

That's fine for scoped_refptrs, but not fine for shared_ptrs. What's the guidance on how to properly fix that so the transition to shared_ptrs is painless if it should happen?

Should we have something like

scoped_refptr<Bar> share_bar() { return bar_; }

alongside get_bar()?

[Peter Kasting]

On Fri, Feb 19, 2016 at 11:14 AM, Gabriel Charette <g...@chromium.org> wrote:

The last bullet perhaps needs to be more contrastful : s/takes a ref/may take a ref/ ?

That seems to make the text more vague and confusing.  What do you mean by "may take a ref"?  To me this reads the same as my rule + the (unwanted) extra proviso "or you can pass as scoped_refptr<T> if maybe someday in the future someone might want to take a ref".

Should the last bullet also explicitly state that "the function should call std::move(t) to take ownership". Otherwise people could respect the rule but still incur an unnecessary ref bump via the temporary argument if its ref is copied instead of taken.

I don't think this is the right place for such guidance, as it's not really about function parameter declarations, and more just a general-purpose "how do I use std::move with scoped_refptr" tip, to be applied anywhere applicable in the codebase.  I'd like to keep this text narrowly focused on just the one topic.

PK

[Gabriel Charette]

On Sun, Feb 21, 2016 at 4:58 PM Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Feb 19, 2016 at 11:14 AM, Gabriel Charette <g...@chromium.org> wrote:

The last bullet perhaps needs to be more contrastful : s/takes a ref/may take a ref/ ?

That seems to make the text more vague and confusing.  What do you mean by "may take a ref"?  To me this reads the same as my rule + the (unwanted) extra proviso "or you can pass as scoped_refptr<T> if maybe someday in the future someone might want to take a ref".

Okay, I don't feel strongly, it's just that "takes a ref" implies that it does, but the method may decide not to keep the ref it was given. Whether it keeps it or not is independent of the style hence why I was trying to articulate a rule that generically supports both use cases.

Should the last bullet also explicitly state that "the function should call std::move(t) to take ownership". Otherwise people could respect the rule but still incur an unnecessary ref bump via the temporary argument if its ref is copied instead of taken.

I don't think this is the right place for such guidance, as it's not really about function parameter declarations, and more just a general-purpose "how do I use std::move with scoped_refptr" tip, to be applied anywhere applicable in the codebase.  I'd like to keep this text narrowly focused on just the one topic.

I see your point but the rule currently has guidance about std::move for the caller. As such it feels like guidance on how to write the entire flow wouldn't hurt (and one more std::move example is a good thing as well IMO as devs aren't yet very familiar with the concept).

PK

[Dana Jansens]

+1 all of the above.

 

 

Antoine

 

David

[Peter Kasting]

I have updated the coding style guidelines with approximately the text suggested.  I also tweaked the smart pointer guidelines doc a bit.

What remains to be done here:

* Remove const scoped_ptr<T>& wherever possible.

* Remove const scoped_refptr<T>& wherever possible.

* Make scoped_refptr<T>::scoped_refptr(T*) explicit.

Would someone like to file bugs and/or take ownership of these?

PK

[Václav Brožek]

Thanks a lot Peter!

I filed the bugs (noted in-line below), although I currently cannot commit to fixing them.

On Tue, 23 Feb 2016 at 03:07 Peter Kasting <pkas...@chromium.org> wrote:

I have updated the coding style guidelines with approximately the text suggested.  I also tweaked the smart pointer guidelines doc a bit.

What remains to be done here:

* Remove const scoped_ptr<T>& wherever possible.

http://crbug.com/589043 

* Remove const scoped_refptr<T>& wherever possible.

http://crbug.com/589044 

* Make scoped_refptr<T>::scoped_refptr(T*) explicit.

http://crbug.com/589048 

[Peter Kasting]

On Tue, Feb 23, 2016 at 2:46 AM, Václav Brožek <va...@chromium.org> wrote:

Thanks a lot Peter!

I filed the bugs (noted in-line below), although I currently cannot commit to fixing them.

Thanks for this.

I did get private mail from a non-committer expressing interest in doing some of this work.  That seems like a good prompt for me to note that:

(1) Anyone is free to tackle bits of the issues below, even if you don't fix the whole codebase.  Incremental improvements are always welcome.

(2) Mechanical changes like this can be a good opportunity for non-committers, in particular, to get some experience with the contribution process on the way towards requesting commit rights.  Take your time and make sure the changes you make are the right ones -- we've had problems in the past with zealous but inexperienced contributors sending in fixes for these sorts of issues that actually introduce bugs -- and this can be a stepping stone to more substantive changes.

If you do start looking to fix some of these, of course, note on the bugs in question what parts of the codebase you're looking at, to avoid duplication of effort.

PK

[Nicolas Zea]

On Tue, Feb 23, 2016 at 3:00 AM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Feb 23, 2016 at 2:46 AM, Václav Brožek <va...@chromium.org> wrote:

Thanks a lot Peter!

I filed the bugs (noted in-line below), although I currently cannot commit to fixing them.

Thanks for this.

I did get private mail from a non-committer expressing interest in doing some of this work.  That seems like a good prompt for me to note that:

(1) Anyone is free to tackle bits of the issues below, even if you don't fix the whole codebase.  Incremental improvements are always welcome.

(2) Mechanical changes like this can be a good opportunity for non-committers, in particular, to get some experience with the contribution process on the way towards requesting commit rights.  Take your time and make sure the changes you make are the right ones -- we've had problems in the past with zealous but inexperienced contributors sending in fixes for these sorts of issues that actually introduce bugs -- and this can be a stepping stone to more substantive changes.

On a related note, is the GoodFirstBug label still in use? Seems like that would apply here.

[Thiago Farina]

On Tue, Feb 23, 2016 at 4:18 PM, Nicolas Zea <z...@chromium.org> wrote:

On Tue, Feb 23, 2016 at 3:00 AM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Feb 23, 2016 at 2:46 AM, Václav Brožek <va...@chromium.org> wrote:

Thanks a lot Peter!

I filed the bugs (noted in-line below), although I currently cannot commit to fixing them.

Thanks for this.

I did get private mail from a non-committer expressing interest in doing some of this work.  That seems like a good prompt for me to note that:

(1) Anyone is free to tackle bits of the issues below, even if you don't fix the whole codebase.  Incremental improvements are always welcome.

(2) Mechanical changes like this can be a good opportunity for non-committers, in particular, to get some experience with the contribution process on the way towards requesting commit rights.  Take your time and make sure the changes you make are the right ones -- we've had problems in the past with zealous but inexperienced contributors sending in fixes for these sorts of issues that actually introduce bugs -- and this can be a stepping stone to more substantive changes.

On a related note, is the GoodFirstBug label still in use? Seems like that would apply here.

 

Yes, I think so. Hotlist=GoodFirstBug in https://bugs.chromium.org/p/chromium/issues/list still seems to return results.

--

Thiago Farina

[Václav Brožek]

Indeed, Hotlist=GoodFirstBug is available and starting contributors actually use it to find good bugs. There is a caveat -- sometimes this label leads people to believe that the asked changes are trivial.

I'm fine with adding that to the bugs I filed, but we need to be prepared to double-check the CLs for having the intended effect. (I know, each such CL will get reviewed, but sometimes the reviewers might lack the context of the bug. This is not a hypothetical issue, I've been bitten in http://crbug.com/393155.)

Cheers,
Vaclav

--

[Timo Reimann]

The Smart Pointer Guidelines contain a few sections that look quite related to the updated code style rules: When do we use each smart pointer, What are the calling conventions involving different kinds of pointers, and What about passing or returning a smart pointer by reference. Should the two possibly be merged or linked together?

I also wonder whether all of the guidelines are still completely valid. For instance, they generally discourage the usage of scoped_refptr<>. As far as I could tell, that argument hasn't been mentioned throughout this thread. Does it still hold?

[Dana Jansens]

On Wed, Feb 24, 2016 at 11:26 AM, 'Timo Reimann' via Chromium-dev <chromi...@chromium.org> wrote:

The Smart Pointer Guidelines contain a few sections that look quite related to the updated code style rules: When do we use each smart pointer, What are the calling conventions involving different kinds of pointers, and What about passing or returning a smart pointer by reference. Should the two possibly be merged or linked together?

I also wonder whether all of the guidelines are still completely valid. For instance, they generally discourage the usage of scoped_refptr<>. As far as I could tell, that argument hasn't been mentioned throughout this thread. Does it still hold?

Yes, don't use ref counting. :)

[Peter Kasting]

On Wed, Feb 24, 2016 at 11:26 AM, 'Timo Reimann' via Chromium-dev <chromi...@chromium.org> wrote:

The Smart Pointer Guidelines contain a few sections that look quite related to the updated code style rules: When do we use each smart pointer, What are the calling conventions involving different kinds of pointers, and What about passing or returning a smart pointer by reference. Should the two possibly be merged or linked together?

I did full review and rewrite passes through both docs earlier this week.  I think the two are complementary.  The code style guidelines assume you know how to use smart pointers and simply tell you what the conventions are for passing them as arguments.  The smart pointer guidelines doc gives complete background on smart pointers in general and the ones in our codebase, and provides more illustrative examples to flesh out the details in the style guide.

When I did my rewrites, I excised a bit of the smart pointer guidelines to avoid overlapping the code style doc too much.

I also wonder whether all of the guidelines are still completely valid. For instance, they generally discourage the usage of scoped_refptr<>. As far as I could tell, that argument hasn't been mentioned throughout this thread. Does it still hold?

Yes.  For the most part, there are better ways to go than refcounting, for the reasons summarized in that doc.

PK