Add move support for scoped_refptr.

Add move support for scoped_refptr.

scoped_ptr has
- move constructor.
- move assignment operator.
- .Pass().

This CL adds the same support for scoped_refptr.
The main benefit is reducing unnecessary reference counter updates.

// Example 1

class Foo {
  // Without .Pass(), a copy constructor would be called, which
  // increments and decrements the reference counter unnecessarily.
  // With .Pass(), a move constructor is called which doesn't update
  // the reference counter
  Foo(scoped_refptr<T> t) : t_(t.Pass()) {}

  const scoped_refptr<T> t_;
};

// Example 2

scoped_refptr<Foo> func() {
  return scoped_refptr<Foo>(new Foo());
}

main() {
  scoped_refptr<Foo> foo;

  // The following would be done by a copy assignment operator before,
  // but now a move assignment operator will be used instead.
  foo = func();
}

// Example 3

class Foo : public base::RefCountedThreadSafe<Foo> {
  ...
}

void func(scoped_refptr<Foo> foo) {
  ...
}

main() {
  scoped_refptr<Foo> foo(...);

  // Because it's moved, the reference counter won't be updated.
  massage_loop->PostTask(FROM_HERE,
      base::Bind(&func, base::Passed(foo)));
}

Committed: https://crrev.com/aa3927982b7cb0032db3f06d89d1c0cfd6479a8b
Cr-Commit-Position: refs/heads/master@{#324910}

[Kibeom Kim (inactive), 2015-04-09 23:37:24]

kkimlabs@chromium.org changed reviewers:
+ danakj@chromium.org, dcheng@chromium.org

[Ryan Sleevi, 2015-04-09 23:47:49]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-04-09 23:47:50]

Drive by:

Case 1 won't cause churn because of RVO
Case 2 is working as intended. You shouldn't be passing
refcounted-non-thread-safe objects across threads, period.

[dcheng, 2015-04-09 23:50:06]

dcheng@chromium.org changed reviewers:
- rsleevi@chromium.org

[dcheng, 2015-04-09 23:50:07]

CCing some other people who might have thoughts.

Also, we should see how far away C++11 library support is, to make the Pass() ->
std::move() conversion a bit easier.

https://codereview.chromium.org/1076953002/diff/20001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1076953002/diff/20001/base/memory/ref_counted...
base/memory/ref_counted.h:338: r.ptr_ = nullptr;
I think this doesn't work for a self-move assignment.

[Matt Giuca, 2015-04-10 00:02:10]

Not a fan of this change. Having move semantics on a refcounted pointer doesn't
make a lot of sense to me, because movement implies ownership transfer, and the
whole point of a scoped_refptr is there is no specific owner (i.e., it is not
*yours* to move).

> Case 1 won't cause churn because of RVO

To expand on this a bit: I think what Ryan is saying is that in your Example 1,
you aren't actually improving performance here because RVO
(return-value-optimization) means that the compiler will create the
scoped_refptr object directly in the |foo| variable. Although there maybe other
situations in which having a move constructor improves performance.

> Case 2 is working as intended. You shouldn't be passing
> refcounted-non-thread-safe objects across threads, period.

Yeah and I think this feature is just going to create more confusion about
thread safety. Even if you can guarantee that base::Passed is going to mean the
reference count doesn't get incremented or decremented throughout the PostTask
process, this is only safe if that was the only reference. I get that you *can*
make it safe, but it sends the wrong message to readers of the code. You're
saying "I have a thing that is specifically designed to have multiple owners and
be non-thread-safe. And I am going to ensure it has exactly 1 owner and pass it
across threads." It's a recipe for confusion and mistakes.

I think the correct way around this is either to make it RefCountedThreadSafe,
or not use a scoped_refptr. Or, as we've been discussing elsewhere, wrap it in a
scoped_refptr to pass across threads.

I would be cool with having a move constructor on scoped_refptr (just for
potential performance improvements), but not with having a Pass() method.

[Kibeom Kim (inactive), 2015-04-10 00:09:14]

On 2015/04/10 00:02:10, Matt Giuca wrote:
> Not a fan of this change. Having move semantics on a refcounted pointer
doesn't
> make a lot of sense to me, because movement implies ownership transfer, and
the
> whole point of a scoped_refptr is there is no specific owner (i.e., it is not
> *yours* to move).
> 
> > Case 1 won't cause churn because of RVO
> 
> To expand on this a bit: I think what Ryan is saying is that in your Example
1,
> you aren't actually improving performance here because RVO
> (return-value-optimization) means that the compiler will create the
> scoped_refptr object directly in the |foo| variable. Although there maybe
other
> situations in which having a move constructor improves performance.
> 

I could be wrong, but I thought that RVO doesn't apply to assignment operator,
only copy constructor.

> > Case 2 is working as intended. You shouldn't be passing
> > refcounted-non-thread-safe objects across threads, period.
> 
> Yeah and I think this feature is just going to create more confusion about
> thread safety. Even if you can guarantee that base::Passed is going to mean
the
> reference count doesn't get incremented or decremented throughout the PostTask
> process, this is only safe if that was the only reference. I get that you
*can*
> make it safe, but it sends the wrong message to readers of the code. You're
> saying "I have a thing that is specifically designed to have multiple owners
and
> be non-thread-safe. And I am going to ensure it has exactly 1 owner and pass
it
> across threads." It's a recipe for confusion and mistakes.
> 
> I think the correct way around this is either to make it RefCountedThreadSafe,
> or not use a scoped_refptr. Or, as we've been discussing elsewhere, wrap it in
a
> scoped_refptr to pass across threads.
> 
> I would be cool with having a move constructor on scoped_refptr (just for
> potential performance improvements), but not with having a Pass() method.

Yeah I think this CL is also fine without a Pass() method.

[Matt Giuca, 2015-04-10 00:11:42]

mgiuca@chromium.org changed reviewers:
- mgiuca@chromium.org

[Kibeom Kim (inactive), 2015-04-10 00:17:45]

btw, std::shared_ptr seems to have move constructor and move assignment
operator.

20.7.2.2.1
template<class Y> shared_ptr(const shared_ptr<Y>& r) noexcept;
shared_ptr(shared_ptr&& r) noexcept;
template<class Y> shared_ptr(shared_ptr<Y>&& r) noexcept;

20.7.2.2.3
shared_ptr& operator=(shared_ptr&& r) noexcept;
template<class Y> shared_ptr& operator=(shared_ptr<Y>&& r) noexcept;

[Ryan Sleevi, 2015-04-10 00:19:02]

On 2015/04/10 00:09:14, Kibeom Kim wrote:
> I could be wrong, but I thought that RVO doesn't apply to assignment operator,
> only copy constructor.

Rather than citing the spec points and finer nuances, I'll just point you to
https://msdn.microsoft.com/en-us/library/ms364057%28v=vs.80%29.aspx for the
state-of-the-art 10 years ago, and how RVO/NVRO totally applies. Note that NVRO
was actually first introduced back ~91, so uh... it's been around for a while :)

[dcheng, 2015-04-10 00:19:04]

On 2015/04/10 at 00:02:10, mgiuca wrote:
> Not a fan of this change. Having move semantics on a refcounted pointer
doesn't make a lot of sense to me, because movement implies ownership transfer,
and the whole point of a scoped_refptr is there is no specific owner (i.e., it
is not *yours* to move).
> 
> > Case 1 won't cause churn because of RVO
> 
> To expand on this a bit: I think what Ryan is saying is that in your Example
1, you aren't actually improving performance here because RVO
(return-value-optimization) means that the compiler will create the
scoped_refptr object directly in the |foo| variable. Although there maybe other
situations in which having a move constructor improves performance.
> 
> > Case 2 is working as intended. You shouldn't be passing
> > refcounted-non-thread-safe objects across threads, period.
> 
> Yeah and I think this feature is just going to create more confusion about
thread safety. Even if you can guarantee that base::Passed is going to mean the
reference count doesn't get incremented or decremented throughout the PostTask
process, this is only safe if that was the only reference. I get that you *can*
make it safe, but it sends the wrong message to readers of the code. You're
saying "I have a thing that is specifically designed to have multiple owners and
be non-thread-safe. And I am going to ensure it has exactly 1 owner and pass it
across threads." It's a recipe for confusion and mistakes.
> 
> I think the correct way around this is either to make it RefCountedThreadSafe,
or not use a scoped_refptr. Or, as we've been discussing elsewhere, wrap it in a
scoped_refptr to pass across threads.
> 
> I would be cool with having a move constructor on scoped_refptr (just for
potential performance improvements), but not with having a Pass() method.

The flip side to this argument is Blink, which does have a moveable refptr type
for performance. There are certain things that will churn without a moveable
scoped_refptr. For example:

Foo f(base::MessageLoop::message_loop_proxy())

Of course, we may simply decide we don't care.

I do think it is weird if we have a move constructor without Pass(). How would
you get an rvalue reference to a scoped_refptr without it?

[jamesr, 2015-04-10 00:22:34]

On 2015/04/10 00:19:04, dcheng wrote:
> The flip side to this argument is Blink, which does have a moveable refptr
type
> for performance.

Blink uses move-aware containers.  Chromium does on some platforms but not on
Android (or mac)

> I do think it is weird if we have a move constructor without Pass(). How would
> you get an rvalue reference to a scoped_refptr without it?

Same way you get an rvalue ref w/out std::move.  you can get one as a prvalue:

foo(SomeExpressionThatProducesScopedRefptr());

or as an xvalue (in a return statement)

[Matt Giuca, 2015-04-10 00:29:21]

Yeah, I just ran an experiment with Clang 3.4 and GCC 4.8.2 (haven't looked at
MSVC). Kibeom is right for this particular example. There is churn (no RVO) as
you are using an assignment operator, not a constructor. If you add a move
assignment operator, it will use that instead of the copy assignment operator.

So there is a performance benefit to this. I'm also coming around about having
Pass(). I guess Pass() is just our way of having std::move() before we get C++11
library support, so the two kind of go hand-in-hand.

Basically, it makes sense from a performance standpoint to have scoped_refptr
with move constructor/assignment and therefore with a Pass() method. But my
problem is with the intended use case (Example 2); this should not be used to
pass a non-thread-safe ref count across threads.

[danakj, 2015-04-10 00:44:16]

On Thu, Apr 9, 2015 at 5:02 PM, <mgiuca@chromium.org> wrote:

> Not a fan of this change. Having move semantics on a refcounted pointer
> doesn't
> make a lot of sense to me, because movement implies ownership transfer,
> and the
> whole point of a scoped_refptr is there is no specific owner (i.e., it is
> not
> *yours* to move).


Without reading everything yet. I fairly strongly support the idea of a
movable scoped_refptr. You're passing your reference, not total ownership.


>
>
>  Case 1 won't cause churn because of RVO
>>
>
> To expand on this a bit: I think what Ryan is saying is that in your
> Example 1,
> you aren't actually improving performance here because RVO
> (return-value-optimization) means that the compiler will create the
> scoped_refptr object directly in the |foo| variable. Although there maybe
> other
> situations in which having a move constructor improves performance.
>
>  Case 2 is working as intended. You shouldn't be passing
>> refcounted-non-thread-safe objects across threads, period.
>>
>
> Yeah and I think this feature is just going to create more confusion about
> thread safety. Even if you can guarantee that base::Passed is going to
> mean the
> reference count doesn't get incremented or decremented throughout the
> PostTask
> process, this is only safe if that was the only reference. I get that you
> *can*
> make it safe, but it sends the wrong message to readers of the code. You're
> saying "I have a thing that is specifically designed to have multiple
> owners and
> be non-thread-safe. And I am going to ensure it has exactly 1 owner and
> pass it
> across threads." It's a recipe for confusion and mistakes.
>
> I think the correct way around this is either to make it
> RefCountedThreadSafe,
> or not use a scoped_refptr. Or, as we've been discussing elsewhere, wrap
> it in a
> scoped_refptr to pass across threads.
>
> I would be cool with having a move constructor on scoped_refptr (just for
> potential performance improvements), but not with having a Pass() method.
>
> https://codereview.chromium.org/1076953002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kibeom Kim (inactive), 2015-04-10 02:42:07]

ptal. revised example 2.

https://codereview.chromium.org/1076953002/diff/20001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1076953002/diff/20001/base/memory/ref_counted...
base/memory/ref_counted.h:280: scoped_refptr(const scoped_refptr<T>& r) :
ptr_(r.ptr_) {
random Q: why do we need this? isn't #286 sufficient?

https://codereview.chromium.org/1076953002/diff/20001/base/memory/ref_counted...
base/memory/ref_counted.h:338: r.ptr_ = nullptr;
On 2015/04/09 23:50:07, dcheng wrote:
> I think this doesn't work for a self-move assignment.

Done.

[Kibeom Kim (inactive), 2015-04-10 02:42:14]

The CQ bit was checked by kkimlabs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-10 02:43:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076953002/40001

[Kibeom Kim (inactive), 2015-04-10 02:56:13]

The CQ bit was checked by kkimlabs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-10 02:56:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076953002/60001

[commit-bot: I haz the power, 2015-04-10 03:15:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-10 03:15:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Kibeom Kim (inactive), 2015-04-10 05:51:25]

The CQ bit was checked by kkimlabs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-10 05:52:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076953002/80001

[commit-bot: I haz the power, 2015-04-10 06:01:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-10 06:01:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[danakj, 2015-04-10 19:44:24]

You might also mention a third example

class Foo {
  Foo(scoped_refptr<T> t) : t_(t.Pass()) {}

  const scoped_refptr<T> t_;
};

https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counte...
base/memory/ref_counted.h:335: scoped_refptr<T>(r.Pass()).swap(*this);
This is:
temp.ptr_ = r.ptr_;
r.ptr_ = nullptr;
T* p = ptr_;
ptr_ = temp.ptr_;
temp.ptr_ = p;

You could do this as:
T* p = r.ptr_;
r.ptr_ = nullptr;
ptr_ = p;

And get the same thing?

[danakj, 2015-04-10 20:14:32]

https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counte...
File base/memory/ref_counted_unittest.cc (right):

https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counte...
base/memory/ref_counted_unittest.cc:319: ScopedRefPtrCount *raw = new
ScopedRefPtrCount();
Can you add some tests with conversion where T != U for the templated type?

[Kibeom Kim (inactive), 2015-04-10 20:32:15]

On 2015/04/10 19:44:24, danakj wrote:
> You might also mention a third example
> 
> class Foo {
>   Foo(scoped_refptr<T> t) : t_(t.Pass()) {}
> 
>   const scoped_refptr<T> t_;
> };
> 

Done. Yeah I think it's good to add a move constructor example. I inserted it as
a first example.

>
https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counted.h
> File base/memory/ref_counted.h (right):
> 
>
https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counte...
> base/memory/ref_counted.h:335: scoped_refptr<T>(r.Pass()).swap(*this);
> This is:
> temp.ptr_ = r.ptr_;
> r.ptr_ = nullptr;
> T* p = ptr_;
> ptr_ = temp.ptr_;
> temp.ptr_ = p;
> 
> You could do this as:
> T* p = r.ptr_;
> r.ptr_ = nullptr;
> ptr_ = p;
> 
> And get the same thing?

I did something similar in patchset 2 but dcheng@ pointed out that it wouldn't
work with assigning itself :( .

[danakj, 2015-04-10 20:34:03]

On Fri, Apr 10, 2015 at 1:32 PM, <kkimlabs@chromium.org> wrote:

> On 2015/04/10 19:44:24, danakj wrote:
>
>> You might also mention a third example
>>
>
>  class Foo {
>>    Foo(scoped_refptr<T> t) : t_(t.Pass()) {}
>>
>
>     const scoped_refptr<T> t_;
>> };
>>
>
>
> Done. Yeah I think it's good to add a move constructor example. I inserted
> it as
> a first example.
>
>
> https://codereview.chromium.org/1076953002/diff/100001/
> base/memory/ref_counted.h
>
>> File base/memory/ref_counted.h (right):
>>
>
>
> https://codereview.chromium.org/1076953002/diff/100001/
> base/memory/ref_counted.h#newcode335
>
>> base/memory/ref_counted.h:335: scoped_refptr<T>(r.Pass()).swap(*this);
>> This is:
>> temp.ptr_ = r.ptr_;
>> r.ptr_ = nullptr;
>> T* p = ptr_;
>> ptr_ = temp.ptr_;
>> temp.ptr_ = p;
>>
>
>  You could do this as:
>> T* p = r.ptr_;
>> r.ptr_ = nullptr;
>> ptr_ = p;
>>
>
>  And get the same thing?
>>
>
> I did something similar in patchset 2 but dcheng@ pointed out that it
> wouldn't
> work with assigning itself :( .
>

Ya I saw, but this should work though, no? It would assign null to itself,
then p?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kibeom Kim (inactive), 2015-04-12 07:35:14]

I noticed that I also need to take care of base/win/scoped_comptr.h too. I'll
fix it in the next patchset.

https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counte...
base/memory/ref_counted.h:335: scoped_refptr<T>(r.Pass()).swap(*this);
On 2015/04/10 19:44:24, danakj wrote:
> This is:
> temp.ptr_ = r.ptr_;
> r.ptr_ = nullptr;
> T* p = ptr_;
> ptr_ = temp.ptr_;
> temp.ptr_ = p;
> 
> You could do this as:
> T* p = r.ptr_;
> r.ptr_ = nullptr;
> ptr_ = p;
> 
> And get the same thing?

Yeah you're right. it also needs |if (ptr_) Release(ptr_);| to be identical.
(corresponds to the temp destructor)

T* p = r.get();
r.ptr_ = nullptr;
if (ptr_)
  Release(ptr_);
ptr_ = p;

https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counte...
File base/memory/ref_counted_unittest.cc (right):

https://codereview.chromium.org/1076953002/diff/100001/base/memory/ref_counte...
base/memory/ref_counted_unittest.cc:319: ScopedRefPtrCount *raw = new
ScopedRefPtrCount();
On 2015/04/10 20:14:32, danakj wrote:
> Can you add some tests with conversion where T != U for the templated type?

Done.

[dcheng, 2015-04-13 03:45:38]

https://codereview.chromium.org/1076953002/diff/140001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1076953002/diff/140001/base/memory/ref_counte...
base/memory/ref_counted.h:343: ptr_ = p;
I think this version is not safe against use-after-frees. One (rather contrived)
example:

struct Self : public RefCounted<Self> {
  Self() : ptr_(this);
  scoped_refptr<Self> ptr_;
 protected:
  friend class RefCounted<Self>;
  virtual ~Self() {}
};

int main() {
  Self* s = new Self;
  scoped_refptr<Self> s2 = new Self;
  s.ptr_ = s2.Pass();
}

Using the original idiom of moving into a temporary and swapping also matches
shared_ptr's implementation in libc++
(https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L4548), etc
too.

[Kibeom Kim (inactive), 2015-04-13 09:57:53]

https://codereview.chromium.org/1076953002/diff/140001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1076953002/diff/140001/base/memory/ref_counte...
base/memory/ref_counted.h:343: ptr_ = p;
On 2015/04/13 03:45:38, dcheng wrote:
> I think this version is not safe against use-after-frees. One (rather
contrived)
> example:
> 
> struct Self : public RefCounted<Self> {
>   Self() : ptr_(this);
>   scoped_refptr<Self> ptr_;
>  protected:
>   friend class RefCounted<Self>;
>   virtual ~Self() {}
> };
> 
> int main() {
>   Self* s = new Self;
>   scoped_refptr<Self> s2 = new Self;
>   s.ptr_ = s2.Pass();
> }
> 
> Using the original idiom of moving into a temporary and swapping also matches
> shared_ptr's implementation in libc++
> (https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L4548), etc
> too.

Done. Added a simplified test case.

ScopedRefPtrToSelf* check = new ScopedRefPtrToSelf();
check->self_ptr_ = scoped_refptr<ScopedRefPtrToSelf>();


Btw, also spec mentions swap directly:

20.7.2.2.3
template<class Y> shared_ptr& operator=(shared_ptr<Y>&& r) noexcept;
  Effects: Equivalent to shared_ptr(std::move(r)).swap(*this).
  Returns: *this

[danakj, 2015-04-13 16:13:23]

Thanks for the additional test kkimlabs, and thanks for pointing out the
self-reference dcheng :)

LGTM

https://codereview.chromium.org/1076953002/diff/160001/base/memory/ref_counte...
File base/memory/ref_counted_unittest.cc (right):

https://codereview.chromium.org/1076953002/diff/160001/base/memory/ref_counte...
base/memory/ref_counted_unittest.cc:135: check->self_ptr_ =
scoped_refptr<ScopedRefPtrToSelf>();
can you leave a comment explaining that if operator= is implemented incorrectly,
check will be freed before check->self_ptr_ is assigned?

[dcheng, 2015-04-13 17:20:45]

Should we update the guidance on passing scoped_refptr<T>? When should one use
const scoped_refptr<T>& vs scoped_refptr<T> + Pass()?

[danakj, 2015-04-13 17:34:19]

I think we should send a PSA once this has landed and stuck, and introduce
people to the concept of using Pass() scoped_refptr (explaining its passing
ownership of the single reference, and maybe give examples comparing it to
PassRefPtr/RefPtr in blink).

We can let people know that const scoped_refptr<T>& still works for passing
things around. But that passing scoped_refptr<T> by value and using Pass() can
lead to better performance, or expression of intent.

[Kibeom Kim (inactive), 2015-04-13 18:37:59]

https://codereview.chromium.org/1076953002/diff/160001/base/memory/ref_counte...
File base/memory/ref_counted_unittest.cc (right):

https://codereview.chromium.org/1076953002/diff/160001/base/memory/ref_counte...
base/memory/ref_counted_unittest.cc:135: check->self_ptr_ =
scoped_refptr<ScopedRefPtrToSelf>();
On 2015/04/13 16:13:23, danakj (away until april 19) wrote:
> can you leave a comment explaining that if operator= is implemented
incorrectly,
> check will be freed before check->self_ptr_ is assigned?

Done.

[Kibeom Kim (inactive), 2015-04-13 18:38:03]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-04-13 18:38:04]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1076953002/#ps180001
(title: "add comment for check->self_ptr_ = scoped_refptr<ScopedRefPtrToSelf>();
test")

[commit-bot: I haz the power, 2015-04-13 18:39:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076953002/180001

[commit-bot: I haz the power, 2015-04-13 20:49:53]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-13 20:50:44]

Patchset 10 (id:??) landed as
https://crrev.com/aa3927982b7cb0032db3f06d89d1c0cfd6479a8b
Cr-Commit-Position: refs/heads/master@{#324910}

[Kibeom Kim (inactive), 2015-04-14 01:37:36]

On 2015/04/13 17:20:45, dcheng wrote:
> Should we update the guidance on passing scoped_refptr<T>? When should one use
> const scoped_refptr<T>& vs scoped_refptr<T> + Pass()?

I don't see any reason to copy when moving is an option. The moved
scoped_refptr<> will become nullptr, and I think one should always move when
that side effect is OK in that context.


On 2015/04/13 17:34:19, danakj (away until april 19) wrote:
> I think we should send a PSA once this has landed and stuck, and introduce
> people to the concept of using Pass() scoped_refptr (explaining its passing
> ownership of the single reference, and maybe give examples comparing it to
> PassRefPtr/RefPtr in blink).
> 
> We can let people know that const scoped_refptr<T>& still works for passing
> things around. But that passing scoped_refptr<T> by value and using Pass() can
> lead to better performance, or expression of intent.

Yeah I'll write a PSA shortly.