Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2274793002:
Added move constructor to PassRefPtr. (Closed)
|
Created:
4 years, 4 months ago by Bugs Nash Modified:
4 years, 3 months ago Reviewers:
Yuta Kitamura CC:
blink-reviews, blink-reviews-wtf_chromium.org, chromium-reviews, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded move constructor to PassRefPtr.
This is the first step in making PassRefPtr move only,
in preparation for removal of PassRefPtr.
BUG=640449
Committed: https://crrev.com/c42809bbfc31acfe4a3d665515b5e0fa81ce1f30
Cr-Commit-Position: refs/heads/master@{#415123}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed reviewer comments #
Total comments: 2
Patch Set 3 : Addressed reviewer comments #
Messages
Total messages: 28 (16 generated)
The CQ bit was checked by bugsnash@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Added move constructor to PassRefPtr. This is the first step in making PassRefPtr move only BUG=640449 ========== to ========== Added move constructor to PassRefPtr. This is the first step in making PassRefPtr move only, in preparation for removal of PassRefPtr. BUG=640449 ==========
bugsnash@chromium.org changed reviewers: + yutak@chromium.org
This move constructor is not strictly needed as it is the same as the copy constructor, which is called instead when passing an rvalue and there is no move constructor
The CQ bit was unchecked by bugsnash@chromium.org
Message was sent while issue was closed.
Can you add a unit test? https://codereview.chromium.org/2274793002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/PassRefPtr.h (right): https://codereview.chromium.org/2274793002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PassRefPtr.h:73: PassRefPtr(const PassRefPtr&& o) : m_ptr(o.leakRef()) {} Remove "const" here. It's confusing to have "const T&&" for any type T, because an rvalue reference represents a temporary value that is not stored anywhere, and it's almost always non-const. In the first place, leakRef() really shouldn't be marked const, as it modifies m_ptr. But I don't feel it worth fixing because we are trying to kill PassRefPtr.
Message was sent while issue was closed.
Oh, was this closed?
Message was sent while issue was closed.
On 2016/08/24 at 08:26:53, yutak wrote: > Oh, was this closed? Sorry I closed this when I thought that it wasn't necessary because it was the same as the copy constructor that would be the fallback. Will reopen.
The CQ bit was checked by bugsnash@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/24 at 08:26:22, yutak wrote: > Can you add a unit test? > > https://codereview.chromium.org/2274793002/diff/1/third_party/WebKit/Source/w... > File third_party/WebKit/Source/wtf/PassRefPtr.h (right): > > https://codereview.chromium.org/2274793002/diff/1/third_party/WebKit/Source/w... > third_party/WebKit/Source/wtf/PassRefPtr.h:73: PassRefPtr(const PassRefPtr&& o) : m_ptr(o.leakRef()) {} > Remove "const" here. It's confusing to have "const T&&" for any type T, because > an rvalue reference represents a temporary value that is not stored anywhere, > and it's almost always non-const. > > In the first place, leakRef() really shouldn't be marked const, as it modifies > m_ptr. But I don't feel it worth fixing because we are trying to kill PassRefPtr. Unit test added
https://codereview.chromium.org/2274793002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/PassRefPtr.h (right): https://codereview.chromium.org/2274793002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/PassRefPtr.h:73: PassRefPtr(const PassRefPtr&& o) : m_ptr(o.leakRef()) {} On 2016/08/24 at 08:26:22, Yuta Kitamura wrote: > Remove "const" here. It's confusing to have "const T&&" for any type T, because > an rvalue reference represents a temporary value that is not stored anywhere, > and it's almost always non-const. > > In the first place, leakRef() really shouldn't be marked const, as it modifies > m_ptr. But I don't feel it worth fixing because we are trying to kill PassRefPtr. Done.
NB: I bypassed the presubmit check to avoid errors due to having local PassRefPtr variables. Given that's what we're testing, I figured it was ok.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
LGTM with nits https://codereview.chromium.org/2274793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/PassRefPtrTest.cpp (right): https://codereview.chromium.org/2274793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PassRefPtrTest.cpp:5: #include "PassRefPtr.h" nit: We spell out the include path fully; no relative paths. https://codereview.chromium.org/2274793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/PassRefPtrTest.cpp:20: PassRefPtr<RefCountedClass> newPassRefPtr = PassRefPtr<RefCountedClass>(std::move(oldPassRefPtr)); nit: This actually causes a move construction AND an assignment. You probably just want a move construction, like this: PassRefPtr<...> newPassRefPtr(std::move(oldPassRefPtr));
The CQ bit was checked by bugsnash@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by bugsnash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org Link to the patchset: https://codereview.chromium.org/2274793002/#ps40001 (title: "Addressed reviewer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Added move constructor to PassRefPtr. This is the first step in making PassRefPtr move only, in preparation for removal of PassRefPtr. BUG=640449 ========== to ========== Added move constructor to PassRefPtr. This is the first step in making PassRefPtr move only, in preparation for removal of PassRefPtr. BUG=640449 Committed: https://crrev.com/c42809bbfc31acfe4a3d665515b5e0fa81ce1f30 Cr-Commit-Position: refs/heads/master@{#415123} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c42809bbfc31acfe4a3d665515b5e0fa81ce1f30 Cr-Commit-Position: refs/heads/master@{#415123} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
