Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1653023003:
Specialize IsMoveOnlyType<T> for vectors. (Closed)
|
Created:
4 years, 10 months ago by dcheng Modified:
4 years, 10 months ago Reviewers:
danakj CC:
chromium-reviews, mdempsky, tzik, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpecialize IsMoveOnlyType<T> for vectors.
Once VC++2015 is the default compiler for Windows, IsMoveOnlyType<T> can
be improved to work generically for all types that are movable but not
copyable. However, this check depends on the copy constructor being
inaccessible, which is problematic for STL containers. A STL container
containing a move-only type still has an accessible copy constructor,
but attempting to use the copy constructor will trigger a compile error.
In order to fix that, IsMoveOnlyType<T> now has a specialization for
std::vector, and will treat a std::vector as move-only if the elements
are move-only.
BUG=554289
Committed: https://crrev.com/53b4cea9f4e7c510b68569003296345989e3bbca
Cr-Commit-Position: refs/heads/master@{#372885}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fix a few things #
Total comments: 6
Patch Set 3 : Address comments #
Messages
Total messages: 22 (8 generated)
dcheng@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne... base/callback_internal.h:14: #include <vector> Unfortunately, doing this means more opportunities for IWYU violations, since callback.h is used pretty widely. But I don't think there's any way around this.
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
Does this mean that this would have to be done for basically all STL containers that may contain a move only type? https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne... base/callback_internal.h:127: IsMoveOnlyType<typename std::vector<T, Allocator>::value_type>::value; FWIW, I think "typename std::vector<T, Allocator>::value_type" is an alias for "T"
Description was changed from ========== Specialize IsMoveOnlyType<T> for vectors. Once VC++2015 is the default compiler for Windows, IsMoveOnlyType<T> can be improved to work generically for all types that are movable but not copyable. However, this check depends on the copy constructor being inaccessible, which is problematic for STL containers. A STL container containing a move-only type still has an accessible copy constructor, but attempting to use the copy constructor will trigger a compile error. In order to fix that, IsMoveOnlyType<T> now has a specialization for std::vector, and will treat a std::vector as move-only if the elements are move-only. BUG=554289 ========== to ========== Specialize IsMoveOnlyType<T> for vectors. Once VC++2015 is the default compiler for Windows, IsMoveOnlyType<T> can be improved to work generically for all types that are movable but not copyable. However, this check depends on the copy constructor being inaccessible, which is problematic for STL containers. A STL container containing a move-only type still has an accessible copy constructor, but attempting to use the copy constructor will trigger a compile error. In order to fix that, IsMoveOnlyType<T> now has a specialization for std::vector, and will treat a std::vector as move-only if the elements are move-only. BUG=554289 ==========
vmpstr@chromium.org changed reviewers: - vmpstr@chromium.org
On 2016/02/01 at 21:36:16, vmpstr wrote: > Does this mean that this would have to be done for basically all STL containers that may contain a move only type? Yes. =( https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne... base/callback_internal.h:127: IsMoveOnlyType<typename std::vector<T, Allocator>::value_type>::value; On 2016/02/01 at 21:36:16, vmpstr wrote: > FWIW, I think "typename std::vector<T, Allocator>::value_type" is an alias for "T" The argument for using value_type is for consistency with associative containers like map and unordered_map: we'd probably have another specialization for std::pair, so we don't have to duplicate "if K or V is move-only, then entire map is move only".
On Mon, Feb 1, 2016 at 2:16 PM, <dcheng@chromium.org> wrote: > On 2016/02/01 at 21:36:16, vmpstr wrote: > > Does this mean that this would have to be done for basically all STL > containers that may contain a move only type? > > Yes. =( > > Let's ask c-users if there's something better we can do.. > https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h > File base/callback_internal.h (right): > https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne... > base/callback_internal.h:127 <https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne...>: IsMoveOnlyType<typename std::vector<T, > Allocator>::value_type>::value; > On 2016/02/01 at 21:36:16, vmpstr wrote: > > FWIW, I think "typename std::vector<T, Allocator>::value_type" is an > alias for "T" > > The argument for using value_type is for consistency with associative > containers like map and unordered_map: we'd probably have another > specialization for std::pair, so we don't have to duplicate "if K or V > is move-only, then entire map is move only". > https://codereview.chromium.org/1653023003/ > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/02/01 at 22:18:37, danakj wrote: > On Mon, Feb 1, 2016 at 2:16 PM, <dcheng@chromium.org> wrote: > > > On 2016/02/01 at 21:36:16, vmpstr wrote: > > > Does this mean that this would have to be done for basically all STL > > containers that may contain a move only type? > > > > Yes. =( > > > > Let's ask c-users if there's something better we can do.. > Yep, I'm already writing a mail. That being said, until we get VC++ 2015, we definitely need something like this anyway. Otherwise, migration away from ScopedVector will be blocked on the fact that the C++11 way to write this simply won't work with base::Callback. > > https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h > > File base/callback_internal.h (right): > > https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne... > > base/callback_internal.h:127 <https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne...>: IsMoveOnlyType<typename std::vector<T, > > Allocator>::value_type>::value; > > On 2016/02/01 at 21:36:16, vmpstr wrote: > > > FWIW, I think "typename std::vector<T, Allocator>::value_type" is an > > alias for "T" > > > > The argument for using value_type is for consistency with associative > > containers like map and unordered_map: we'd probably have another > > specialization for std::pair, so we don't have to duplicate "if K or V > > is move-only, then entire map is move only". > > https://codereview.chromium.org/1653023003/ > > > > > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
On 2016/02/01 22:23:58, dcheng wrote: > On 2016/02/01 at 22:18:37, danakj wrote: > > On Mon, Feb 1, 2016 at 2:16 PM, <mailto:dcheng@chromium.org> wrote: > > > > > On 2016/02/01 at 21:36:16, vmpstr wrote: > > > > Does this mean that this would have to be done for basically all STL > > > containers that may contain a move only type? > > > > > > Yes. =( > > > > > > Let's ask c-users if there's something better we can do.. > > > > Yep, I'm already writing a mail. That being said, until we get VC++ 2015, we > definitely need something like this anyway. Otherwise, migration away from > ScopedVector will be blocked on the fact that the C++11 way to write this simply > won't work with base::Callback. FWIW, http://stackoverflow.com/questions/18404108/issue-with-is-copy-constructible implies that this is gonna be an issue with is_copy_constructible, but the accepted answer has a neat way of getting around it (by specializing) :P > > > > https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h > > > File base/callback_internal.h (right): > > > > https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne... > > > base/callback_internal.h:127 > <https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne...>: > IsMoveOnlyType<typename std::vector<T, > > > Allocator>::value_type>::value; > > > On 2016/02/01 at 21:36:16, vmpstr wrote: > > > > FWIW, I think "typename std::vector<T, Allocator>::value_type" is an > > > alias for "T" > > > > > > The argument for using value_type is for consistency with associative > > > containers like map and unordered_map: we'd probably have another > > > specialization for std::pair, so we don't have to duplicate "if K or V > > > is move-only, then entire map is move only". > > > https://codereview.chromium.org/1653023003/ > > > > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email to mailto:chromium-reviews+unsubscribe@chromium.org. > >
On 2016/02/01 at 22:25:17, vmpstr wrote: > On 2016/02/01 22:23:58, dcheng wrote: > > On 2016/02/01 at 22:18:37, danakj wrote: > > > On Mon, Feb 1, 2016 at 2:16 PM, <mailto:dcheng@chromium.org> wrote: > > > > > > > On 2016/02/01 at 21:36:16, vmpstr wrote: > > > > > Does this mean that this would have to be done for basically all STL > > > > containers that may contain a move only type? > > > > > > > > Yes. =( > > > > > > > > Let's ask c-users if there's something better we can do.. > > > > > > > Yep, I'm already writing a mail. That being said, until we get VC++ 2015, we > > definitely need something like this anyway. Otherwise, migration away from > > ScopedVector will be blocked on the fact that the C++11 way to write this simply > > won't work with base::Callback. > > FWIW, http://stackoverflow.com/questions/18404108/issue-with-is-copy-constructible implies that this is gonna be an issue with is_copy_constructible, but the accepted answer has a neat way of getting around it (by specializing) :P Sadly, that's what c-users said as well: this is a known defect in the design of STL containers, and the best way to work around it is with template specializations like this =/ > > > > > > > https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h > > > > File base/callback_internal.h (right): > > > > > > https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne... > > > > base/callback_internal.h:127 > > <https://codereview.chromium.org/1653023003/diff/1/base/callback_internal.h#ne...>: > > IsMoveOnlyType<typename std::vector<T, > > > > Allocator>::value_type>::value; > > > > On 2016/02/01 at 21:36:16, vmpstr wrote: > > > > > FWIW, I think "typename std::vector<T, Allocator>::value_type" is an > > > > alias for "T" > > > > > > > > The argument for using value_type is for consistency with associative > > > > containers like map and unordered_map: we'd probably have another > > > > specialization for std::pair, so we don't have to duplicate "if K or V > > > > is move-only, then entire map is move only". > > > > https://codereview.chromium.org/1653023003/ > > > > > > > > > > > > > > -- > > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:chromium-reviews+unsubscribe@chromium.org. > > >
LGTMM https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.... base/callback_internal.h:120: struct IsMoveOnlyType<std::unique_ptr<T, D>> : public std::true_type {}; nit: let's leave out redundant public here and below https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.... base/callback_internal.h:123: // element type is move-only. Leave a note that it ignores the Allocator?
LGTM too
https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.... base/callback_internal.h:126: : public IsMoveOnlyType<typename std::vector<T, Allocator>::value_type> {}; vlad's right though that this is the same as : IsMoveOnlyType<T> {} which is way easier to read, tho less copy-paste friendly. I think I like that better.
The CQ bit was checked by dcheng@chromium.org
https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.h File base/callback_internal.h (right): https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.... base/callback_internal.h:120: struct IsMoveOnlyType<std::unique_ptr<T, D>> : public std::true_type {}; On 2016/02/02 at 01:38:34, danakj wrote: > nit: let's leave out redundant public here and below Done. https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.... base/callback_internal.h:123: // element type is move-only. On 2016/02/02 at 01:38:33, danakj wrote: > Leave a note that it ignores the Allocator? Done. https://codereview.chromium.org/1653023003/diff/20001/base/callback_internal.... base/callback_internal.h:126: : public IsMoveOnlyType<typename std::vector<T, Allocator>::value_type> {}; On 2016/02/02 at 01:39:21, danakj wrote: > vlad's right though that this is the same as > > : IsMoveOnlyType<T> {} > > which is way easier to read, tho less copy-paste friendly. I think I like that better. OK, sure. I had imagined that we could use some generic programming to enable this for most STL containers without having to explicitly specify each one, but that might be a bit too clever.
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/1653023003/#ps40001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1653023003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1653023003/40001
Message was sent while issue was closed.
Description was changed from ========== Specialize IsMoveOnlyType<T> for vectors. Once VC++2015 is the default compiler for Windows, IsMoveOnlyType<T> can be improved to work generically for all types that are movable but not copyable. However, this check depends on the copy constructor being inaccessible, which is problematic for STL containers. A STL container containing a move-only type still has an accessible copy constructor, but attempting to use the copy constructor will trigger a compile error. In order to fix that, IsMoveOnlyType<T> now has a specialization for std::vector, and will treat a std::vector as move-only if the elements are move-only. BUG=554289 ========== to ========== Specialize IsMoveOnlyType<T> for vectors. Once VC++2015 is the default compiler for Windows, IsMoveOnlyType<T> can be improved to work generically for all types that are movable but not copyable. However, this check depends on the copy constructor being inaccessible, which is problematic for STL containers. A STL container containing a move-only type still has an accessible copy constructor, but attempting to use the copy constructor will trigger a compile error. In order to fix that, IsMoveOnlyType<T> now has a specialization for std::vector, and will treat a std::vector as move-only if the elements are move-only. BUG=554289 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Specialize IsMoveOnlyType<T> for vectors. Once VC++2015 is the default compiler for Windows, IsMoveOnlyType<T> can be improved to work generically for all types that are movable but not copyable. However, this check depends on the copy constructor being inaccessible, which is problematic for STL containers. A STL container containing a move-only type still has an accessible copy constructor, but attempting to use the copy constructor will trigger a compile error. In order to fix that, IsMoveOnlyType<T> now has a specialization for std::vector, and will treat a std::vector as move-only if the elements are move-only. BUG=554289 ========== to ========== Specialize IsMoveOnlyType<T> for vectors. Once VC++2015 is the default compiler for Windows, IsMoveOnlyType<T> can be improved to work generically for all types that are movable but not copyable. However, this check depends on the copy constructor being inaccessible, which is problematic for STL containers. A STL container containing a move-only type still has an accessible copy constructor, but attempting to use the copy constructor will trigger a compile error. In order to fix that, IsMoveOnlyType<T> now has a specialization for std::vector, and will treat a std::vector as move-only if the elements are move-only. BUG=554289 Committed: https://crrev.com/53b4cea9f4e7c510b68569003296345989e3bbca Cr-Commit-Position: refs/heads/master@{#372885} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/53b4cea9f4e7c510b68569003296345989e3bbca Cr-Commit-Position: refs/heads/master@{#372885} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
