Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1454993003:
Add a move constructor and move assignment operator to scoped_ptr. (Closed)
DescriptionAdd a move constructor and move assignment operator to scoped_ptr.
Previously, scoped_ptr only had a conversion constructor with a
scoped_ptr<U, E> rvalue reference argument. Since this isn't an
actual move constructor, this prevented clang's pessimizing move
warning from trigger on code like this:
scoped_ptr<int> F() { scoped_ptr<int> i; return std::move(i); }
In addition, the conversion constructor is now disabled if <U, E>
are not convertible to <T, D> respectively. This is important so:
1. type_traits like std::is_convertible don't incorrectly treat
scoped_ptr<Base> as convertible to scoped_ptr<Derived>.
2. Code like this compiles:
void F(scoped_ptr<int>);
void F(scoped_ptr<Base>);
F(scoped_ptr<Derived>());
For symmetry, scoped_ptr also has a move assignment operator now,
and the conversion assignment operator has been updated to match
the conditions when the conversion constructor is disabled via
SFINAE.
BUG=557438
Committed: https://crrev.com/ecdd662b725c21d5536defc8d6be81ed781caa07
Cr-Commit-Position: refs/heads/master@{#360737}
Patch Set 1 #Patch Set 2 : Add a real move constructor #
Total comments: 5
Patch Set 3 : . #Patch Set 4 : Add header #
Total comments: 14
Patch Set 5 : Address comments #Patch Set 6 : More template magic #
Total comments: 4
Patch Set 7 : Moar comments #Patch Set 8 : Close enough #Messages
Total messages: 54 (18 generated)
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:337: scoped_ptr(scoped_ptr<U, V>&& other, I wish clang-format did a better job wrapping template code, this is always a guessing game of find-the-closing-angle-bracket. https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:339: std::is_convertible<typename scoped_ptr<U, V>::Pointer, Can you explain to me why you used enable_if here? You only compare the Pointer, and not the deleter, and it would return true of U::Pointer == |this type|::Pointer. We didn't need it before cuz the impl_(&other.impl_) requires them to convert, I think? Why now?
Sorry I haven't had more time to hack on this. Are you going to adopt this into the scoped_refptr patch? I'm happy to just close this issue out. https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:339: std::is_convertible<typename scoped_ptr<U, V>::Pointer, On 2015/11/18 at 23:39:34, danakj (behind on reviews) wrote: > Can you explain to me why you used enable_if here? You only compare the Pointer, and not the deleter, and it would return true of U::Pointer == |this type|::Pointer. > > We didn't need it before cuz the impl_(&other.impl_) requires them to convert, I think? Why now? I don't actually know the reasoning behind this: it's just what the standard says. Specifically, in 20.7.1.2.1.19: Remarks: This constructor shall not participate in overload resolution unless: — unique_ptr<U, E>::pointer is implicitly convertible to pointer, — U is not an array type, and — either D is a reference type and E is the same type as D, or D is not a reference type and E is implicitly convertible to D. I'm sure there's a good reason for this, but I don't know what it is.
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:339: std::is_convertible<typename scoped_ptr<U, V>::Pointer, On 2015/11/19 00:23:24, dcheng wrote: > On 2015/11/18 at 23:39:34, danakj (behind on reviews) wrote: > > Can you explain to me why you used enable_if here? You only compare the > Pointer, and not the deleter, and it would return true of U::Pointer == |this > type|::Pointer. > > > > We didn't need it before cuz the impl_(&other.impl_) requires them to convert, > I think? Why now? > > I don't actually know the reasoning behind this: it's just what the standard > says. Specifically, in 20.7.1.2.1.19: > Remarks: This constructor shall not participate in overload resolution unless: > — unique_ptr<U, E>::pointer is implicitly convertible to pointer, > — U is not an array type, and > — either D is a reference type and E is the same type as D, or D is not a > reference type and E is > implicitly convertible to D. > > I'm sure there's a good reason for this, but I don't know what it is. Is it possible to enable_if in the template list instead?
https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:339: std::is_convertible<typename scoped_ptr<U, V>::Pointer, On 2015/11/19 at 00:37:18, vmpstr wrote: > On 2015/11/19 00:23:24, dcheng wrote: > > On 2015/11/18 at 23:39:34, danakj (behind on reviews) wrote: > > > Can you explain to me why you used enable_if here? You only compare the > > Pointer, and not the deleter, and it would return true of U::Pointer == |this > > type|::Pointer. > > > > > > We didn't need it before cuz the impl_(&other.impl_) requires them to convert, > > I think? Why now? > > > > I don't actually know the reasoning behind this: it's just what the standard > > says. Specifically, in 20.7.1.2.1.19: > > Remarks: This constructor shall not participate in overload resolution unless: > > — unique_ptr<U, E>::pointer is implicitly convertible to pointer, > > — U is not an array type, and > > — either D is a reference type and E is the same type as D, or D is not a > > reference type and E is > > implicitly convertible to D. > > > > I'm sure there's a good reason for this, but I don't know what it is. > > Is it possible to enable_if in the template list instead? Any particular reason to prefer that? I guess the style ban on default args?
On 2015/11/19 00:38:57, dcheng wrote: > https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.h > File base/memory/scoped_ptr.h (right): > > https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.... > base/memory/scoped_ptr.h:339: std::is_convertible<typename scoped_ptr<U, > V>::Pointer, > On 2015/11/19 at 00:37:18, vmpstr wrote: > > On 2015/11/19 00:23:24, dcheng wrote: > > > On 2015/11/18 at 23:39:34, danakj (behind on reviews) wrote: > > > > Can you explain to me why you used enable_if here? You only compare the > > > Pointer, and not the deleter, and it would return true of U::Pointer == > |this > > > type|::Pointer. > > > > > > > > We didn't need it before cuz the impl_(&other.impl_) requires them to > convert, > > > I think? Why now? > > > > > > I don't actually know the reasoning behind this: it's just what the standard > > > says. Specifically, in 20.7.1.2.1.19: > > > Remarks: This constructor shall not participate in overload resolution > unless: > > > — unique_ptr<U, E>::pointer is implicitly convertible to pointer, > > > — U is not an array type, and > > > — either D is a reference type and E is the same type as D, or D is not > a > > > reference type and E is > > > implicitly convertible to D. > > > > > > I'm sure there's a good reason for this, but I don't know what it is. > > > > Is it possible to enable_if in the template list instead? > > Any particular reason to prefer that? I guess the style ban on default args? That and doesn't that become a constructor that can take a second argument of a void*? :) It's kind of awkward.
On 2015/11/19 00:39:43, vmpstr wrote: > On 2015/11/19 00:38:57, dcheng wrote: > > https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.h > > File base/memory/scoped_ptr.h (right): > > > > > https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.... > > base/memory/scoped_ptr.h:339: std::is_convertible<typename scoped_ptr<U, > > V>::Pointer, > > On 2015/11/19 at 00:37:18, vmpstr wrote: > > > On 2015/11/19 00:23:24, dcheng wrote: > > > > On 2015/11/18 at 23:39:34, danakj (behind on reviews) wrote: > > > > > Can you explain to me why you used enable_if here? You only compare the > > > > Pointer, and not the deleter, and it would return true of U::Pointer == > > |this > > > > type|::Pointer. > > > > > > > > > > We didn't need it before cuz the impl_(&other.impl_) requires them to > > convert, > > > > I think? Why now? > > > > > > > > I don't actually know the reasoning behind this: it's just what the > standard > > > > says. Specifically, in 20.7.1.2.1.19: > > > > Remarks: This constructor shall not participate in overload resolution > > unless: > > > > — unique_ptr<U, E>::pointer is implicitly convertible to pointer, > > > > — U is not an array type, and > > > > — either D is a reference type and E is the same type as D, or D is > not > > a > > > > reference type and E is > > > > implicitly convertible to D. > > > > > > > > I'm sure there's a good reason for this, but I don't know what it is. > > > > > > Is it possible to enable_if in the template list instead? > > > > Any particular reason to prefer that? I guess the style ban on default args? > > That and doesn't that become a constructor that can take a second argument of a > void*? :) It's kind of awkward. To immediately clarify, I think it's fine if that's the only choice for the constructor, but if it's templated then why not do it in a template? Or you can enable_if<..., unique_ptr<U, E>>::type&& for the first parameter
On 2015/11/19 00:40:43, vmpstr wrote: > On 2015/11/19 00:39:43, vmpstr wrote: > > On 2015/11/19 00:38:57, dcheng wrote: > > > > https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.h > > > File base/memory/scoped_ptr.h (right): > > > > > > > > > https://codereview.chromium.org/1454993003/diff/20001/base/memory/scoped_ptr.... > > > base/memory/scoped_ptr.h:339: std::is_convertible<typename scoped_ptr<U, > > > V>::Pointer, > > > On 2015/11/19 at 00:37:18, vmpstr wrote: > > > > On 2015/11/19 00:23:24, dcheng wrote: > > > > > On 2015/11/18 at 23:39:34, danakj (behind on reviews) wrote: > > > > > > Can you explain to me why you used enable_if here? You only compare > the > > > > > Pointer, and not the deleter, and it would return true of U::Pointer == > > > |this > > > > > type|::Pointer. > > > > > > > > > > > > We didn't need it before cuz the impl_(&other.impl_) requires them to > > > convert, > > > > > I think? Why now? > > > > > > > > > > I don't actually know the reasoning behind this: it's just what the > > standard > > > > > says. Specifically, in 20.7.1.2.1.19: > > > > > Remarks: This constructor shall not participate in overload resolution > > > unless: > > > > > — unique_ptr<U, E>::pointer is implicitly convertible to pointer, > > > > > — U is not an array type, and > > > > > — either D is a reference type and E is the same type as D, or D is > > not > > > a > > > > > reference type and E is > > > > > implicitly convertible to D. > > > > > > > > > > I'm sure there's a good reason for this, but I don't know what it is. > > > > > > > > Is it possible to enable_if in the template list instead? > > > > > > Any particular reason to prefer that? I guess the style ban on default args? > > > > That and doesn't that become a constructor that can take a second argument of > a > > void*? :) It's kind of awkward. > > To immediately clarify, I think it's fine if that's the only choice for the > constructor, but if it's templated then why not do it in a template? Or you can > enable_if<..., unique_ptr<U, E>>::type&& for the first parameter Hmm, nevermind.. I think the template list one is probably worse since you can either bypass it or clang gives a less-than-ideal message.
Add a move constructor and move assignment operator to scoped_ptr.
Previously, scoped_ptr only had a conversion constructor with a
scoped_ptr<U, E> rvalue reference argument. Since this isn't an
actual move constructor, this prevented clang's pessimizing move
warning from trigger on code like this:
scoped_ptr<int> F() { scoped_ptr<int> i; return std::move(i); }
In addition, the conversion constructor is now disabled if <U, E>
are not convertible to <T, D> respectively. This is important so:
1. type_traits like std::is_convertible don't incorrectly treat
scoped_ptr<Derived> as convertible to scoped_ptr<Unrelated>.
For symmetry, scoped_ptr also has a move assignment operator now,
and the conversion assignment operator has been updated to match
the conditions when the conversion constructor is disabled via
SFINAE.
BUG=557438
Oops, I thought I was editing the issue description. Please ignore >_<
Description was changed from
==========
Simple repro for no pessimizing move warning.
BUG=557438
==========
to
==========
Add a move constructor and move assignment operator to scoped_ptr.
Previously, scoped_ptr only had a conversion constructor with a
scoped_ptr<U, E> rvalue reference argument. Since this isn't an
actual move constructor, this prevented clang's pessimizing move
warning from trigger on code like this:
scoped_ptr<int> F() { scoped_ptr<int> i; return std::move(i); }
In addition, the conversion constructor is now disabled if <U, E>
are not convertible to <T, D> respectively. This is important so:
1. type_traits like std::is_convertible don't incorrectly treat
scoped_ptr<Derived> as convertible to scoped_ptr<Unrelated>.
For symmetry, scoped_ptr also has a move assignment operator now,
and the conversion assignment operator has been updated to match
the conditions when the conversion constructor is disabled via
SFINAE.
BUG=557438
==========
Description was changed from
==========
Add a move constructor and move assignment operator to scoped_ptr.
Previously, scoped_ptr only had a conversion constructor with a
scoped_ptr<U, E> rvalue reference argument. Since this isn't an
actual move constructor, this prevented clang's pessimizing move
warning from trigger on code like this:
scoped_ptr<int> F() { scoped_ptr<int> i; return std::move(i); }
In addition, the conversion constructor is now disabled if <U, E>
are not convertible to <T, D> respectively. This is important so:
1. type_traits like std::is_convertible don't incorrectly treat
scoped_ptr<Derived> as convertible to scoped_ptr<Unrelated>.
For symmetry, scoped_ptr also has a move assignment operator now,
and the conversion assignment operator has been updated to match
the conditions when the conversion constructor is disabled via
SFINAE.
BUG=557438
==========
to
==========
Add a move constructor and move assignment operator to scoped_ptr.
Previously, scoped_ptr only had a conversion constructor with a
scoped_ptr<U, E> rvalue reference argument. Since this isn't an
actual move constructor, this prevented clang's pessimizing move
warning from trigger on code like this:
scoped_ptr<int> F() { scoped_ptr<int> i; return std::move(i); }
In addition, the conversion constructor is now disabled if <U, E>
are not convertible to <T, D> respectively. This is important so:
1. type_traits like std::is_convertible don't incorrectly treat
scoped_ptr<Base> as convertible to scoped_ptr<Derived>.
2. Code like this compiles:
void F(scoped_ptr<int>);
void F(scoped_ptr<Base>);
F(scoped_ptr<Derived>());
For symmetry, scoped_ptr also has a move assignment operator now,
and the conversion assignment operator has been updated to match
the conditions when the conversion constructor is disabled via
SFINAE.
BUG=557438
==========
Description was changed from
==========
Add a move constructor and move assignment operator to scoped_ptr.
Previously, scoped_ptr only had a conversion constructor with a
scoped_ptr<U, E> rvalue reference argument. Since this isn't an
actual move constructor, this prevented clang's pessimizing move
warning from trigger on code like this:
scoped_ptr<int> F() { scoped_ptr<int> i; return std::move(i); }
In addition, the conversion constructor is now disabled if <U, E>
are not convertible to <T, D> respectively. This is important so:
1. type_traits like std::is_convertible don't incorrectly treat
scoped_ptr<Base> as convertible to scoped_ptr<Derived>.
2. Code like this compiles:
void F(scoped_ptr<int>);
void F(scoped_ptr<Base>);
F(scoped_ptr<Derived>());
For symmetry, scoped_ptr also has a move assignment operator now,
and the conversion assignment operator has been updated to match
the conditions when the conversion constructor is disabled via
SFINAE.
BUG=557438
==========
to
==========
Add a move constructor and move assignment operator to scoped_ptr.
Previously, scoped_ptr only had a conversion constructor with a
scoped_ptr<U, E> rvalue reference argument. Since this isn't an
actual move constructor, this prevented clang's pessimizing move
warning from trigger on code like this:
scoped_ptr<int> F() { scoped_ptr<int> i; return std::move(i); }
In addition, the conversion constructor is now disabled if <U, E>
are not convertible to <T, D> respectively. This is important so:
1. type_traits like std::is_convertible don't incorrectly treat
scoped_ptr<Base> as convertible to scoped_ptr<Derived>.
2. Code like this compiles:
void F(scoped_ptr<int>);
void F(scoped_ptr<Base>);
F(scoped_ptr<Derived>());
For symmetry, scoped_ptr also has a move assignment operator now,
and the conversion assignment operator has been updated to match
the conditions when the conversion constructor is disabled via
SFINAE.
BUG=557438
==========
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993003/40001
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
The CQ bit was unchecked by dcheng@chromium.org
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
OK, this should be ready for review. I've confirmed locally that it warns, but obviously I can't check that in. If you feel strongly, I can try to get the no-compile tests working for scoped_ptr again, but this is an area of the code that perpetually bitrots =( https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:293: typename E, I renamed the second template parameter from V to E, since it matches more logically with D -> E. https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:408: using pointer_type = element_type*; This is for consistency, but probably not needed, strictly speaking.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993003/60001
Oh, and I plan on handling scoped_refptr in a separate CL, just to keep things simpler/focused.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
(driiiive byyyy) https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:268: // we <_< https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:326: typename = typename std::enable_if< So, to elaborate on what I was saying earlier, you can bypass this enable_if by explicitly specifying the third template argument. The reason for this is that it's a default value that has the enable if, but if you specify the type the default isn't used. One of the ways to fix it is something along the lines of: template <typename U, typename E, typename std::enable_if<..., int>::type = 0> which makes the enable_if be the actual type of the third parameter (int), which means you can't bypass it even if you specify the third template parameter. The other option of course is what the standard library does with a private type and a second parameter to the ctor. https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:408: using pointer_type = element_type*; On 2015/11/19 20:49:10, dcheng wrote: > This is for consistency, but probably not needed, strictly speaking. FWIW, I think reading std::is_convertible<U*, T*>::value is a lot easier :)
https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:268: // we wrapping broke https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:285: // // ambiguous since both conversion constructors match. nit: capital A https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:293: typename E, On 2015/11/19 20:49:10, dcheng wrote: > I renamed the second template parameter from V to E, since it matches more > logically with D -> E. sgtm https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:408: using pointer_type = element_type*; On 2015/11/19 20:49:10, dcheng wrote: > This is for consistency, but probably not needed, strictly speaking. unique_ptr has ::pointer, not pointer_type. and it has it on both the unique_ptr<T> and unique_ptr<T[]>. https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2562 https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2748
On 2015/11/19 at 21:00:24, vmpstr wrote: > (driiiive byyyy) > > https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.h > File base/memory/scoped_ptr.h (right): > > https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... > base/memory/scoped_ptr.h:268: // we > <_< > > https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... > base/memory/scoped_ptr.h:326: typename = typename std::enable_if< > So, to elaborate on what I was saying earlier, you can bypass this enable_if by explicitly specifying the third template argument. The reason for this is that it's a default value that has the enable if, but if you specify the type the default isn't used. > > One of the ways to fix it is something along the lines of: > > template <typename U, > typename E, > typename std::enable_if<..., int>::type = 0> > > which makes the enable_if be the actual type of the third parameter (int), which means you can't bypass it even if you specify the third template parameter. > > The other option of course is what the standard library does with a private type and a second parameter to the ctor. > > https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... > base/memory/scoped_ptr.h:408: using pointer_type = element_type*; > On 2015/11/19 20:49:10, dcheng wrote: > > This is for consistency, but probably not needed, strictly speaking. > > FWIW, I think reading > > std::is_convertible<U*, T*>::value > > is a lot easier :) So it turns out pointer is there, because of C++11 20.7.1.2.1.3: If the type remove_reference<D>::type::pointer exists, then unique_ptr<T, D>::pointer shall be a synonym for remove_reference<D>::type::pointer. Otherwise unique_ptr<T, D>::pointer shall be a synonym for T*. The type unique_ptr<T, D>::pointer shall satisfy the requirements of NullablePointer I'm not terribly eager to support this, so I'm happy to just get rid of it.
https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:268: // we On 2015/11/19 at 21:03:36, danakj (behind on reviews) wrote: > wrapping broke Oops. https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:285: // // ambiguous since both conversion constructors match. On 2015/11/19 at 21:03:36, danakj (behind on reviews) wrote: > nit: capital A Done. https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:326: typename = typename std::enable_if< On 2015/11/19 at 21:00:24, vmpstr wrote: > So, to elaborate on what I was saying earlier, you can bypass this enable_if by explicitly specifying the third template argument. The reason for this is that it's a default value that has the enable if, but if you specify the type the default isn't used. > > One of the ways to fix it is something along the lines of: > > template <typename U, > typename E, > typename std::enable_if<..., int>::type = 0> > > which makes the enable_if be the actual type of the third parameter (int), which means you can't bypass it even if you specify the third template parameter. > > The other option of course is what the standard library does with a private type and a second parameter to the ctor. This is the only way I could get it to compile: template <typename U, typename E, typename std::enable_if<...>::type* = nullptr> https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:408: using pointer_type = element_type*; On 2015/11/19 at 21:03:36, danakj (behind on reviews) wrote: > On 2015/11/19 20:49:10, dcheng wrote: > > This is for consistency, but probably not needed, strictly speaking. > > unique_ptr has ::pointer, not pointer_type. and it has it on both the unique_ptr<T> and unique_ptr<T[]>. > > https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2562 > https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2748 Given vmpstr@'s feedback and the fact that our pointer type alias isn't quite the same as unique_ptr, I've just removed it.
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993003/100001
https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... base/memory/scoped_ptr.h:326: typename = typename std::enable_if< On 2015/11/19 21:26:04, dcheng wrote: > On 2015/11/19 at 21:00:24, vmpstr wrote: > > So, to elaborate on what I was saying earlier, you can bypass this enable_if > by explicitly specifying the third template argument. The reason for this is > that it's a default value that has the enable if, but if you specify the type > the default isn't used. > > > > One of the ways to fix it is something along the lines of: > > > > template <typename U, > > typename E, > > typename std::enable_if<..., int>::type = 0> > > > > which makes the enable_if be the actual type of the third parameter (int), > which means you can't bypass it even if you specify the third template > parameter. > > > > The other option of course is what the standard library does with a private > type and a second parameter to the ctor. > > This is the only way I could get it to compile: > template <typename U, > typename E, > typename std::enable_if<...>::type* = nullptr> My guess is you didn't put the ", int" part in the enable_if :) But void* works as well as anything else. Thanks.
On 2015/11/19 at 21:28:41, vmpstr wrote: > https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.h > File base/memory/scoped_ptr.h (right): > > https://codereview.chromium.org/1454993003/diff/60001/base/memory/scoped_ptr.... > base/memory/scoped_ptr.h:326: typename = typename std::enable_if< > On 2015/11/19 21:26:04, dcheng wrote: > > On 2015/11/19 at 21:00:24, vmpstr wrote: > > > So, to elaborate on what I was saying earlier, you can bypass this enable_if > > by explicitly specifying the third template argument. The reason for this is > > that it's a default value that has the enable if, but if you specify the type > > the default isn't used. > > > > > > One of the ways to fix it is something along the lines of: > > > > > > template <typename U, > > > typename E, > > > typename std::enable_if<..., int>::type = 0> > > > > > > which makes the enable_if be the actual type of the third parameter (int), > > which means you can't bypass it even if you specify the third template > > parameter. > > > > > > The other option of course is what the standard library does with a private > > type and a second parameter to the ctor. > > > > This is the only way I could get it to compile: > > template <typename U, > > typename E, > > typename std::enable_if<...>::type* = nullptr> > > My guess is you didn't put the ", int" part in the enable_if :) But void* works as well as anything else. Thanks. Ah yes, I missed that you had specified the second template arg.
LGTM https://codereview.chromium.org/1454993003/diff/100001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/100001/base/memory/scoped_ptr... base/memory/scoped_ptr.h:247: typedef T element_type; using here too while you're at it? :) https://codereview.chromium.org/1454993003/diff/100001/base/memory/scoped_ptr... base/memory/scoped_ptr.h:326: std::is_convertible<E, D>::value>::type* = This should be is_assignable here for the deleter https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2655
https://codereview.chromium.org/1454993003/diff/100001/base/memory/scoped_ptr.h File base/memory/scoped_ptr.h (right): https://codereview.chromium.org/1454993003/diff/100001/base/memory/scoped_ptr... base/memory/scoped_ptr.h:247: typedef T element_type; On 2015/11/19 at 21:31:11, danakj (behind on reviews) wrote: > using here too while you're at it? :) Done. https://codereview.chromium.org/1454993003/diff/100001/base/memory/scoped_ptr... base/memory/scoped_ptr.h:326: std::is_convertible<E, D>::value>::type* = On 2015/11/19 at 21:31:11, danakj (behind on reviews) wrote: > This should be is_assignable here for the deleter > > https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L2655 Done.
The CQ bit was checked by dcheng@chromium.org
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/1454993003/#ps110001 (title: "Moar comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993003/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993003/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, no build URL) android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL)
So the bots are not a big fan of this change. It looks like std::is_assignable is in the same bucket as std::underlying_type: it just doesn't work on lots of our platforms =( I'll send an update to the C++11 style guide. In the mean time, maybe is_convertible<> is a good enough approximation?
On Thu, Nov 19, 2015 at 3:01 PM, <dcheng@chromium.org> wrote: > So the bots are not a big fan of this change. It looks like > std::is_assignable > is in the same bucket as std::underlying_type: it just doesn't work on > lots of > our platforms =( > > I'll send an update to the C++11 style guide. In the mean time, maybe > is_convertible<> is a good enough approximation? > Uh.. it's more strict right? It means if operator= would have worked, it might not know. But things that are implicitly convertible can always be assigned. It seems okay for now. -- 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.
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
On 2015/11/19 at 23:03:25, danakj wrote: > On Thu, Nov 19, 2015 at 3:01 PM, <dcheng@chromium.org> wrote: > > > So the bots are not a big fan of this change. It looks like > > std::is_assignable > > is in the same bucket as std::underlying_type: it just doesn't work on > > lots of > > our platforms =( > > > > I'll send an update to the C++11 style guide. In the mean time, maybe > > is_convertible<> is a good enough approximation? > > > > Uh.. it's more strict right? It means if operator= would have worked, it > might not know. But things that are implicitly convertible can always be > assigned. It seems okay for now. > > -- > 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. > I uploaded a version that uses is_convertible for now. I don't think is_convertible implies is_assignable though: you could write a class such that it is convertible but not assignable. Also, I was curious how libstdc++4.6 deals with this. The answer? It doesn't! It also uses the typename = typename std::enable_if<> construct that previous versions of this CL used.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993003/120001
On Thu, Nov 19, 2015 at 4:08 PM, <dcheng@chromium.org> wrote: > On 2015/11/19 at 23:03:25, danakj wrote: > >> On Thu, Nov 19, 2015 at 3:01 PM, <dcheng@chromium.org> wrote: >> > > > So the bots are not a big fan of this change. It looks like >> > std::is_assignable >> > is in the same bucket as std::underlying_type: it just doesn't work on >> > lots of >> > our platforms =( >> > >> > I'll send an update to the C++11 style guide. In the mean time, maybe >> > is_convertible<> is a good enough approximation? >> > >> > > Uh.. it's more strict right? It means if operator= would have worked, it >> might not know. But things that are implicitly convertible can always be >> assigned. It seems okay for now. >> > > -- >> 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. > > > I uploaded a version that uses is_convertible for now. I don't think > is_convertible implies is_assignable though: you could write a class such > that > it is convertible but not assignable. > Really? But wouldn't it just implicitly convert and bind to that operator=? > > Also, I was curious how libstdc++4.6 deals with this. The answer? It > doesn't! > > It also uses the typename = typename std::enable_if<> construct that > previous > versions of this CL used. > Fun. :) Then that seems extra okay. And motivating to use the std lib so we can blame them? :P -- 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 2015/11/20 at 00:39:09, danakj wrote: > On Thu, Nov 19, 2015 at 4:08 PM, <dcheng@chromium.org> wrote: > > > On 2015/11/19 at 23:03:25, danakj wrote: > > > >> On Thu, Nov 19, 2015 at 3:01 PM, <dcheng@chromium.org> wrote: > >> > > > > > So the bots are not a big fan of this change. It looks like > >> > std::is_assignable > >> > is in the same bucket as std::underlying_type: it just doesn't work on > >> > lots of > >> > our platforms =( > >> > > >> > I'll send an update to the C++11 style guide. In the mean time, maybe > >> > is_convertible<> is a good enough approximation? > >> > > >> > > > > Uh.. it's more strict right? It means if operator= would have worked, it > >> might not know. But things that are implicitly convertible can always be > >> assigned. It seems okay for now. > >> > > > > -- > >> 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. > > > > > > I uploaded a version that uses is_convertible for now. I don't think > > is_convertible implies is_assignable though: you could write a class such > > that > > it is convertible but not assignable. > > > > Really? But wouldn't it just implicitly convert and bind to that operator=? dcheng@truffula:~/src/chrome/src$ third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++11 test7.cc dcheng@truffula:~/src/chrome/src$ cat test7.cc #include <iostream> #include <memory> #include <type_traits> struct B { }; struct A { A(const B&) {} A(const A&) {} A& operator=(const A&) = delete; }; int main() { std::cout << std::boolalpha; std::cout << "std::is_convertible<B, A>: " << std::is_convertible<B, A>::value << "\n"; std::cout << "std::is_assignable<A, B>: " << std::is_assignable<A, B>::value << "\n"; } dcheng@truffula:~/src/chrome/src$ ./a.out std::is_convertible<B, A>: true std::is_assignable<A, B>: false > > > > > > Also, I was curious how libstdc++4.6 deals with this. The answer? It > > doesn't! > > > > It also uses the typename = typename std::enable_if<> construct that > > previous > > versions of this CL used. > > > > Fun. :) Then that seems extra okay. And motivating to use the std lib so we > can blame them? :P > > -- > 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. >
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dcheng@chromium.org
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/1454993003/#ps120001 (title: "Close enough")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1454993003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1454993003/120001
Message was sent while issue was closed.
Committed patchset #8 (id:120001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ecdd662b725c21d5536defc8d6be81ed781caa07 Cr-Commit-Position: refs/heads/master@{#360737}
Message was sent while issue was closed.
On Thu, Nov 19, 2015 at 5:58 PM, <dcheng@chromium.org> wrote: > On 2015/11/20 at 00:39:09, danakj wrote: > >> On Thu, Nov 19, 2015 at 4:08 PM, <dcheng@chromium.org> wrote: >> > > > On 2015/11/19 at 23:03:25, danakj wrote: >> > >> >> On Thu, Nov 19, 2015 at 3:01 PM, <dcheng@chromium.org> wrote: >> >> >> > >> > > So the bots are not a big fan of this change. It looks like >> >> > std::is_assignable >> >> > is in the same bucket as std::underlying_type: it just doesn't work >> on >> >> > lots of >> >> > our platforms =( >> >> > >> >> > I'll send an update to the C++11 style guide. In the mean time, maybe >> >> > is_convertible<> is a good enough approximation? >> >> > >> >> >> > >> > Uh.. it's more strict right? It means if operator= would have worked, it >> >> might not know. But things that are implicitly convertible can always >> be >> >> assigned. It seems okay for now. >> >> >> > >> > -- >> >> 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. >> > >> > >> > I uploaded a version that uses is_convertible for now. I don't think >> > is_convertible implies is_assignable though: you could write a class >> such >> > that >> > it is convertible but not assignable. >> > >> > > Really? But wouldn't it just implicitly convert and bind to that operator=? >> > > dcheng@truffula:~/src/chrome/src$ > third_party/llvm-build/Release+Asserts/bin/clang++ -std=c++11 test7.cc > dcheng@truffula:~/src/chrome/src$ cat test7.cc > #include <iostream> > #include <memory> > #include <type_traits> > > struct B { > }; > > struct A { > A(const B&) {} > A(const A&) {} > A& operator=(const A&) = delete; > }; > > int main() { > std::cout << std::boolalpha; > std::cout << "std::is_convertible<B, A>: " << std::is_convertible<B, > A>::value > << "\n"; > std::cout << "std::is_assignable<A, B>: " << std::is_assignable<A, > B>::value > << "\n"; > } > dcheng@truffula:~/src/chrome/src$ ./a.out > std::is_convertible<B, A>: true > std::is_assignable<A, B>: false Ahh = delete, so cheating. Good thinking :) > > > > > > >> > Also, I was curious how libstdc++4.6 deals with this. The answer? It >> > doesn't! >> > >> > It also uses the typename = typename std::enable_if<> construct that >> > previous >> > versions of this CL used. >> > >> > > Fun. :) Then that seems extra okay. And motivating to use the std lib so we >> can blame them? :P >> > > -- >> 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. > > > https://codereview.chromium.org/1454993003/ > -- 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. |
