Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
978783002:
give UniquePersistent full move semantics (Closed)
Descriptiongive UniquePersistent full move semantics
BUG=v8:3669
LOG=Y
Committed: https://crrev.com/6e189f5ae6af20eb9d27f954bae8c5532749fb1b
Cr-Commit-Position: refs/heads/master@{#27004}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : tests #
Total comments: 7
Patch Set 4 : #Messages
Total messages: 20 (8 generated)
svenpanne@chromium.org changed reviewers: + svenpanne@chromium.org
lgtm
The CQ bit was checked by dcarney@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svenpanne@chromium.org Link to the patchset: https://codereview.chromium.org/978783002/#ps40001 (title: "tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/978783002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/3578)
dcarney@chromium.org changed reviewers: + jamesr@chromium.org
it turns out this is anyway disallowed by the styleguide without permission of a styleguide owner, so I added one. jamesr@ this is a feature we would really like for a single class: UniquePersistent, which is effectively a unique_ptr a lot of internal state that needs to be managed. Not being able to use move semantics causes a lot jumping through hoops to have containers of them. Do you know if there is a upcoming discussion about move semantics or is it okay to sneak it in this exceptional case?
The style guide only applies to Chromium proper, not subrepos, but some of the toolchain constraints that apply to Chromium also apply to V8 (or at least the parts of V8 that need to build in chromium). +thakis for OS X https://codereview.chromium.org/978783002/diff/40001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/978783002/diff/40001/build/standalone.gypi#ne... build/standalone.gypi:527: 'CLANG_CXX_LIBRARY': 'libc++', # -stdlib=libc++ We support OS X 10.6 which doesn't support C++11, IIUC https://codereview.chromium.org/978783002/diff/40001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/978783002/diff/40001/include/v8.h#newcode854 include/v8.h:854: V8_INLINE UniquePersistent& operator=(UniquePersistent<S>&& rhs) { operator=(T&&)s should support self assignment, but this appears to blow away the object if |*this| == |rhs|. a safer way to write this would be if (this != &rhs) { TYPE_CHECK .. } return *this https://codereview.chromium.org/978783002/diff/40001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/978783002/diff/40001/test/cctest/test-api.cc#... test/cctest/test-api.cc:3260: vector.push_back(std::move(global)); this won't compile on all platforms that chromium builds on. does this build as part of chromium builds or only v8 builds?
thakis@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/978783002/diff/40001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/978783002/diff/40001/build/standalone.gypi#ne... build/standalone.gypi:527: 'CLANG_CXX_LIBRARY': 'libc++', # -stdlib=libc++ On 2015/03/04 18:52:37, jamesr wrote: > We support OS X 10.6 which doesn't support C++11, IIUC Yes, this won't work in a chromium build. (standalone.gypi isn't used in a chromium build, so this is fine, but if any v8 code that's used in chromium tries to use c++11 library features, things won't work)
I believe everything in v8.h will work in Chromium, though (modulo the self-assignment bug)
thanks for looking at this. we try to keep our code as close to the style guide as possible. i'll discuss this change further with the team https://codereview.chromium.org/978783002/diff/40001/build/standalone.gypi File build/standalone.gypi (right): https://codereview.chromium.org/978783002/diff/40001/build/standalone.gypi#ne... build/standalone.gypi:527: 'CLANG_CXX_LIBRARY': 'libc++', # -stdlib=libc++ On 2015/03/04 18:57:56, Nico wrote: > On 2015/03/04 18:52:37, jamesr wrote: > > We support OS X 10.6 which doesn't support C++11, IIUC > > Yes, this won't work in a chromium build. (standalone.gypi isn't used in a > chromium build, so this is fine, but if any v8 code that's used in chromium > tries to use c++11 library features, things won't work) okay, good to know. I was just trying to get the test working on my machine, so i'll revert this file. https://codereview.chromium.org/978783002/diff/40001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/978783002/diff/40001/include/v8.h#newcode854 include/v8.h:854: V8_INLINE UniquePersistent& operator=(UniquePersistent<S>&& rhs) { good catch, thanks https://codereview.chromium.org/978783002/diff/40001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/978783002/diff/40001/test/cctest/test-api.cc#... test/cctest/test-api.cc:3260: vector.push_back(std::move(global)); On 2015/03/04 18:52:37, jamesr wrote: > this won't compile on all platforms that chromium builds on. does this build as > part of chromium builds or only v8 builds? this test stuff is v8 only. I just wanted to check to see how many of our builders could handle it.
If this were Chromium code, I'd be inclined to allow this use (although if it was actually chromium code I'd tell you to use the macro, but that's obviously not practical to do for this header).
The CQ bit was checked by dcarney@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svenpanne@chromium.org Link to the patchset: https://codereview.chromium.org/978783002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/978783002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6e189f5ae6af20eb9d27f954bae8c5532749fb1b Cr-Commit-Position: refs/heads/master@{#27004} |
