[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added inline comments.
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:161
+ {
+// Though types are different, initialization can be done with `memcpy`.
+int32_t array[1] = {-1};
Quuxplusone wrote:
> vsapsai wrote:
> > Quuxplusone wrote:
> > > I might add "can be, but currently isn't, done with memcpy."
> > >
> > > Here's my other suggested test:
> > >
> > > ```
> > > struct X { int x; };
> > > struct Y { int y; };
> > > struct Z : X, Y { int z; };
> > > {
> > > Z z;
> > > Z *array[1] = { };
> > > // Though the types Z* and Y* are very similar, initialization still
> > > cannot be done with memcpy.
> > > std::vector v(array, array + 1);
> > > assert(v[0] == );
> > > }
> > > ```
> > Thanks, I'll try your `XYZ` test, looks like it should help with my
> > `iostream*/ostream*` struggles.
> LGTM! Ship it!
> (I'm surprised the structs can be defined inside the body of `main()` like
> that, but I have no objection to doing so.)
I've decided not to add "can be, but currently isn't, done with memcpy" because
I don't believe that comment will be kept up to date.
Thanks for all the rounds of review, Arthur.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL350583: [libcxx] Optimize vectors construction of trivial
types from an iterator range… (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D48342?vs=180577=180583#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
Files:
libcxx/trunk/include/memory
libcxx/trunk/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
Index: libcxx/trunk/include/memory
===
--- libcxx/trunk/include/memory
+++ libcxx/trunk/include/memory
@@ -1502,6 +1502,12 @@
typedef typename _Alloc::difference_type type;
};
+template
+struct __is_default_allocator : false_type {};
+
+template
+struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type {};
+
template
struct _LIBCPP_TEMPLATE_VIS allocator_traits
{
@@ -1615,7 +1621,7 @@
static
typename enable_if
<
-(is_same >::value
+(__is_default_allocator::value
|| !__has_construct::value) &&
is_trivially_move_constructible<_Tp>::value,
void
@@ -1640,23 +1646,25 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template ::type,
+ class _RawDestTp = typename remove_const<_DestTp>::type>
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+is_trivially_move_constructible<_DestTp>::value &&
+is_same<_RawSourceTp, _RawDestTp>::value &&
+(__is_default_allocator::value ||
+ !__has_construct::value),
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np * sizeof(_Tp));
+_VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np * sizeof(_DestTp));
__begin2 += _Np;
}
}
@@ -1679,7 +1687,7 @@
static
typename enable_if
<
-(is_same >::value
+(__is_default_allocator::value
|| !__has_construct::value) &&
is_trivially_move_constructible<_Tp>::value,
void
Index: libcxx/trunk/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
===
--- libcxx/trunk/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
+++ libcxx/trunk/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
@@ -146,9 +146,40 @@
#endif
}
+// Initialize a vector with a different value type.
+void test_ctor_with_different_value_type() {
+ {
+// Make sure initialization is performed with each element value, not with
+// a memory blob.
+float array[3] = {0.0f, 1.0f, 2.0f};
+std::vector v(array, array + 3);
+assert(v[0] == 0);
+assert(v[1] == 1);
+assert(v[2] == 2);
+ }
+ struct X { int x; };
+ struct Y { int y; };
+ struct Z : X, Y { int z; };
+ {
+Z z;
+Z *array[1] = { };
+// Though the types Z* and Y* are very similar, initialization still cannot
+// be done with `memcpy`.
+std::vector v(array, array + 1);
+assert(v[0] == );
+ }
+ {
+// Though the types are different, initialization can be done with `memcpy`.
+int32_t array[1] = { -1 };
+std::vector v(array, array + 1);
+assert(v[0] == 4294967295);
+ }
+}
+
int main() {
basic_test_cases();
emplaceable_concept_tests(); // See PR34898
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
Quuxplusone added inline comments.
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:161
+ {
+// Though types are different, initialization can be done with `memcpy`.
+int32_t array[1] = {-1};
vsapsai wrote:
> Quuxplusone wrote:
> > I might add "can be, but currently isn't, done with memcpy."
> >
> > Here's my other suggested test:
> >
> > ```
> > struct X { int x; };
> > struct Y { int y; };
> > struct Z : X, Y { int z; };
> > {
> > Z z;
> > Z *array[1] = { };
> > // Though the types Z* and Y* are very similar, initialization still
> > cannot be done with memcpy.
> > std::vector v(array, array + 1);
> > assert(v[0] == );
> > }
> > ```
> Thanks, I'll try your `XYZ` test, looks like it should help with my
> `iostream*/ostream*` struggles.
LGTM! Ship it!
(I'm surprised the structs can be defined inside the body of `main()` like
that, but I have no objection to doing so.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai updated this revision to Diff 180577.
vsapsai added a comment.
- Test initializing vector of pointers with array of pointers of convertible
type.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
===
--- libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
@@ -146,9 +146,40 @@
#endif
}
+// Initialize a vector with a different value type.
+void test_ctor_with_different_value_type() {
+ {
+// Make sure initialization is performed with each element value, not with
+// a memory blob.
+float array[3] = {0.0f, 1.0f, 2.0f};
+std::vector v(array, array + 3);
+assert(v[0] == 0);
+assert(v[1] == 1);
+assert(v[2] == 2);
+ }
+ struct X { int x; };
+ struct Y { int y; };
+ struct Z : X, Y { int z; };
+ {
+Z z;
+Z *array[1] = { };
+// Though the types Z* and Y* are very similar, initialization still cannot
+// be done with `memcpy`.
+std::vector v(array, array + 1);
+assert(v[0] == );
+ }
+ {
+// Though the types are different, initialization can be done with `memcpy`.
+int32_t array[1] = { -1 };
+std::vector v(array, array + 1);
+assert(v[0] == 4294967295);
+ }
+}
+
int main() {
basic_test_cases();
emplaceable_concept_tests(); // See PR34898
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1502,6 +1502,12 @@
typedef typename _Alloc::difference_type type;
};
+template
+struct __is_default_allocator : false_type {};
+
+template
+struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type {};
+
template
struct _LIBCPP_TEMPLATE_VIS allocator_traits
{
@@ -1615,7 +1621,7 @@
static
typename enable_if
<
-(is_same >::value
+(__is_default_allocator::value
|| !__has_construct::value) &&
is_trivially_move_constructible<_Tp>::value,
void
@@ -1640,23 +1646,25 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template ::type,
+ class _RawDestTp = typename remove_const<_DestTp>::type>
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+is_trivially_move_constructible<_DestTp>::value &&
+is_same<_RawSourceTp, _RawDestTp>::value &&
+(__is_default_allocator::value ||
+ !__has_construct::value),
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np * sizeof(_Tp));
+_VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np * sizeof(_DestTp));
__begin2 += _Np;
}
}
@@ -1679,7 +1687,7 @@
static
typename enable_if
<
-(is_same >::value
+(__is_default_allocator::value
|| !__has_construct::value) &&
is_trivially_move_constructible<_Tp>::value,
void
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added inline comments.
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:161
+ {
+// Though types are different, initialization can be done with `memcpy`.
+int32_t array[1] = {-1};
Quuxplusone wrote:
> I might add "can be, but currently isn't, done with memcpy."
>
> Here's my other suggested test:
>
> ```
> struct X { int x; };
> struct Y { int y; };
> struct Z : X, Y { int z; };
> {
> Z z;
> Z *array[1] = { };
> // Though the types Z* and Y* are very similar, initialization still
> cannot be done with memcpy.
> std::vector v(array, array + 1);
> assert(v[0] == );
> }
> ```
Thanks, I'll try your `XYZ` test, looks like it should help with my
`iostream*/ostream*` struggles.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added inline comments. Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:196 test_ctor_under_alloc(); + test_ctor_with_different_value_type(); } Quuxplusone wrote: > vsapsai wrote: > > Quuxplusone wrote: > > > I suggest that interesting test cases include "array of `int` to vector > > > of `unsigned int`" (trivial, but unimplemented in this patch) and "array > > > of `iostream*` to vector of `ostream*`" (non-trivial because each pointer > > > must be adjusted). > > What is that supposed to test? My `float/int` test is to make sure we have > > `is_same<_RawSourceTp, _RawDestTp>::value` and don't try to `memcpy` > > unrelated types. I've chosen `float` and `int` because it is easier for a > > human to reason about them. > > > > `int` and `unsigned int` are interested for testing for values that are > > outside of common range. But in this case you pay more attention to > > conversion between ints, not to the behaviour of the constructor. That's my > > interpretation but maybe I've missed some of your intentions. > > My `float/int` test is to make sure we [...] don't try to `memcpy` > > unrelated types [when it's unsafe to do so]. > > Right. I suggest two additional tests. The first additional test, > `int/unsigned int`, is to verify whether we `memcpy` unrelated types when it > //is// safe to do so. The second test, `iostream*/ostream*`, is to verify > whether we `memcpy` unrelated types when it is unsafe to `memcpy` them even > though they are both of the same "kind" (both pointer types). > > These will act as regression tests, to make sure that future changes to the > memcpy criteria don't break these cases' behavior (although the first > additional test //is// expected to get more efficient). Added test for `int32_t/uint32_t`. Size is mentioned explicitly to avoid surprises with testing on different architectures. Tested that such initialization isn't using `memcpy` and still slow. Do you know ways to verify pointer adjustment for `iostream*/ostream*` using their public API? Because I found a way to do that with accessing `vector::data()` and mucking with pointers. But I don't like this approach because it seems brittle and specific to vector implementation. I'd rather use stable public API. Currently I don't plan to invest much effort into `iostream*/ostream*` test because I agree it is nice to have but `is_same` part is already covered. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D48342/new/ https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
Quuxplusone added inline comments.
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:161
+ {
+// Though types are different, initialization can be done with `memcpy`.
+int32_t array[1] = {-1};
I might add "can be, but currently isn't, done with memcpy."
Here's my other suggested test:
```
struct X { int x; };
struct Y { int y; };
struct Z : X, Y { int z; };
{
Z z;
Z *array[1] = { };
// Though the types Z* and Y* are very similar, initialization still cannot
be done with memcpy.
std::vector v(array, array + 1);
assert(v[0] == );
}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai updated this revision to Diff 180556.
vsapsai added a comment.
- Test initializing vector of `unsigned int` with array of `int`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
===
--- libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
@@ -146,9 +146,29 @@
#endif
}
+// Initialize a vector with a different value type.
+void test_ctor_with_different_value_type() {
+ {
+// Make sure initialization is performed with each element value, not with
+// a memory blob.
+float array[3] = {0.0f, 1.0f, 2.0f};
+std::vector v(array, array + 3);
+assert(v[0] == 0);
+assert(v[1] == 1);
+assert(v[2] == 2);
+ }
+ {
+// Though types are different, initialization can be done with `memcpy`.
+int32_t array[1] = {-1};
+std::vector v(array, array + 1);
+assert(v[0] == 4294967295);
+ }
+}
+
int main() {
basic_test_cases();
emplaceable_concept_tests(); // See PR34898
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1502,6 +1502,12 @@
typedef typename _Alloc::difference_type type;
};
+template
+struct __is_default_allocator : false_type {};
+
+template
+struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type {};
+
template
struct _LIBCPP_TEMPLATE_VIS allocator_traits
{
@@ -1615,7 +1621,7 @@
static
typename enable_if
<
-(is_same >::value
+(__is_default_allocator::value
|| !__has_construct::value) &&
is_trivially_move_constructible<_Tp>::value,
void
@@ -1640,23 +1646,25 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template ::type,
+ class _RawDestTp = typename remove_const<_DestTp>::type>
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+is_trivially_move_constructible<_DestTp>::value &&
+is_same<_RawSourceTp, _RawDestTp>::value &&
+(__is_default_allocator::value ||
+ !__has_construct::value),
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np * sizeof(_Tp));
+_VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np * sizeof(_DestTp));
__begin2 += _Np;
}
}
@@ -1679,7 +1687,7 @@
static
typename enable_if
<
-(is_same >::value
+(__is_default_allocator::value
|| !__has_construct::value) &&
is_trivially_move_constructible<_Tp>::value,
void
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
ldionne accepted this revision. ldionne added a comment. LGTM, but I suggest you address @Quuxplusone 's concerns regarding the tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D48342/new/ https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
Quuxplusone added inline comments. Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:196 test_ctor_under_alloc(); + test_ctor_with_different_value_type(); } vsapsai wrote: > Quuxplusone wrote: > > I suggest that interesting test cases include "array of `int` to vector of > > `unsigned int`" (trivial, but unimplemented in this patch) and "array of > > `iostream*` to vector of `ostream*`" (non-trivial because each pointer must > > be adjusted). > What is that supposed to test? My `float/int` test is to make sure we have > `is_same<_RawSourceTp, _RawDestTp>::value` and don't try to `memcpy` > unrelated types. I've chosen `float` and `int` because it is easier for a > human to reason about them. > > `int` and `unsigned int` are interested for testing for values that are > outside of common range. But in this case you pay more attention to > conversion between ints, not to the behaviour of the constructor. That's my > interpretation but maybe I've missed some of your intentions. > My `float/int` test is to make sure we [...] don't try to `memcpy` unrelated > types [when it's unsafe to do so]. Right. I suggest two additional tests. The first additional test, `int/unsigned int`, is to verify whether we `memcpy` unrelated types when it //is// safe to do so. The second test, `iostream*/ostream*`, is to verify whether we `memcpy` unrelated types when it is unsafe to `memcpy` them even though they are both of the same "kind" (both pointer types). These will act as regression tests, to make sure that future changes to the memcpy criteria don't break these cases' behavior (although the first additional test //is// expected to get more efficient). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D48342/new/ https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added inline comments. Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:196 test_ctor_under_alloc(); + test_ctor_with_different_value_type(); } Quuxplusone wrote: > I suggest that interesting test cases include "array of `int` to vector of > `unsigned int`" (trivial, but unimplemented in this patch) and "array of > `iostream*` to vector of `ostream*`" (non-trivial because each pointer must > be adjusted). What is that supposed to test? My `float/int` test is to make sure we have `is_same<_RawSourceTp, _RawDestTp>::value` and don't try to `memcpy` unrelated types. I've chosen `float` and `int` because it is easier for a human to reason about them. `int` and `unsigned int` are interested for testing for values that are outside of common range. But in this case you pay more attention to conversion between ints, not to the behaviour of the constructor. That's my interpretation but maybe I've missed some of your intentions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D48342/new/ https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai updated this revision to Diff 180316.
vsapsai added a comment.
- Tweak the test.
- Use for `__construct_range_forward` approach recommended by @ldionne (it
works with C++03).
- Use `__is_default_allocator` for `__construct_forward` and
`__construct_backward`.
Didn't introduce `remove_const` to `__construct_forward` and
`__construct_backward` because they don't allow mixing types.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
Index:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
===
---
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
+++
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
@@ -146,9 +146,20 @@
#endif
}
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+void test_ctor_with_different_value_type() {
+ float array[3] = {0.0f, 1.0f, 2.0f};
+ std::vector v(array, array + 3);
+ assert(v[0] == 0);
+ assert(v[1] == 1);
+ assert(v[2] == 2);
+}
+
int main() {
basic_test_cases();
emplaceable_concept_tests(); // See PR34898
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1502,6 +1502,12 @@
typedef typename _Alloc::difference_type type;
};
+template
+struct __is_default_allocator : false_type {};
+
+template
+struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type {};
+
template
struct _LIBCPP_TEMPLATE_VIS allocator_traits
{
@@ -1615,7 +1621,7 @@
static
typename enable_if
<
-(is_same >::value
+(__is_default_allocator::value
|| !__has_construct::value) &&
is_trivially_move_constructible<_Tp>::value,
void
@@ -1640,23 +1646,25 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template ::type,
+ class _RawDestTp = typename remove_const<_DestTp>::type>
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+is_trivially_move_constructible<_DestTp>::value &&
+is_same<_RawSourceTp, _RawDestTp>::value &&
+(__is_default_allocator::value ||
+ !__has_construct::value),
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1,
_Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1,
_SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np *
sizeof(_Tp));
+_VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np
* sizeof(_DestTp));
__begin2 += _Np;
}
}
@@ -1679,7 +1687,7 @@
static
typename enable_if
<
-(is_same >::value
+(__is_default_allocator::value
|| !__has_construct::value) &&
is_trivially_move_constructible<_Tp>::value,
void
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
===
--- libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
@@ -146,9 +146,20 @@
#endif
}
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+void test_ctor_with_different_value_type() {
+ float array[3] = {0.0f, 1.0f, 2.0f};
+ std::vector v(array, array + 3);
+ assert(v[0] == 0);
+ assert(v[1] == 1);
+ assert(v[2] == 2);
+}
+
int main() {
basic_test_cases();
emplaceable_concept_tests(); // See PR34898
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1502,6 +1502,12 @@
typedef typename _Alloc::difference_type type;
};
+template
+struct __is_default_allocator : false_type {};
+
+template
+struct
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
ldionne added inline comments. Comment at: libcxx/include/memory:1658 +|| !__has_construct::value) && + is_trivially_move_constructible<_DestTp>::value, void Quuxplusone wrote: > Shouldn't this be something like `is_trivially_constructible<_DestTp, > _SourceTp&>`? I mean, we're never doing move-construction here. We're > constructing `_DestTp` from `_SourceTp&`, which is either copy-construction > or non-const-copy-construction, depending on the constness of `_SourceTp`. > > `is_trivially_constructible` has pitfalls in general — > https://quuxplusone.github.io/blog/2018/07/03/trivially-constructible-from/ — > but I think we won't fall into any of those pits as long as we're testing > that `_SourceTp` is cv-qualified `_DestTp`. > Shouldn't this be something like `is_trivially_constructible<_DestTp, > _SourceTp&>`? I think you're right. We should also fix `__construct_forward` and `__construct_backward`, but maybe as part of a separate change since the three are consistently using `is_trivially_move_constructible` (and so there might be a reason for this). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D48342/new/ https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
Quuxplusone added inline comments.
Comment at: libcxx/include/memory:1645
-template
+template
_LIBCPP_INLINE_VISIBILITY
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > Coming at it from a slightly different angle, I would think this is what
> > > we want:
> > >
> > > ```
> > > template > > class _RawSourceTp = typename remove_const<_SourceTp>::type,
> > > class _RawDestTp = typename remove_const<_DestTp>::type>
> > > _LIBCPP_INLINE_VISIBILITY static typename enable_if<
> > >
> > > // We can use memcpy instead of a loop with construct if...
> > > is_trivially_move_constructible<_DestTp>::value &&
> > > // - the Dest is trivially move constructible, and
> > > is_same<_RawSourceTp, _RawDestTp>::value &&
> > > // - both types are the same modulo constness, and either
> > > (__is_default_allocator::value ||
> > > // + the allocator is the default allocator (and we know `construct`
> > > is just placement-new), or
> > > !__has_construct > > const&>::value), // + the allocator does not provide a custom
> > > `construct` method (so we'd fall back to placement-new)
> > > void>::type
> > > __construct_range_forward(allocator_type&, _SourceTp* __begin1,
> > > _SourceTp* __end1, _DestTp*& __begin2)
> > > {
> > > ptrdiff_t _Np = __end1 - __begin1;
> > > if (_Np > 0)
> > > {
> > > _VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np *
> > > sizeof(_DestTp));
> > > __begin2 += _Np;
> > > }
> > > }
> > > ```
> > >
> > > And then we should have
> > >
> > > ```
> > > template
> > > struct __is_default_allocator : false_type { };
> > >
> > > template
> > > struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type { };
> > > ```
> > >
> > > Does this make sense?
> > >
> > > Also, I'm not sure I understand why we use `const_cast` on the
> > > destination type. It seems like we should instead enforce that it is
> > > non-const? But this is a pre-existing thing in the code, this doesn't
> > > affect this review.
> > >
> > I agree that it is wrong to express the check in terms of
> > `is_same>`; it should be expressed in terms
> > of a trait which is satisfied by `std::allocator`-for-any-T. @ldionne
> > expressed it in terms of `__is_default_allocator`. I continue to ask
> > that it be expressed in terms of `__has_trivial_construct > _SourceTp&>`, where libc++ specializes
> > `__has_trivial_construct, ...>` if need be.
> >
> > Orthogonally, the condition `__has_construct > _SourceTp const&>` is wrong because it has an extra `const`. It is
> > conceivable — though of course implausible/pathological — for
> > `construct(T*, T&)` to exist and do something different from `construct(T*,
> > const T&)`.
> > I continue to ask that it be expressed in terms of
> > `__has_trivial_construct`, where libc++
> > specializes `__has_trivial_construct, ...>` if need be.
>
> Would you be OK with us applying this fix and then generalizing
> `__is_default_allocator` into `__has_trivial_construct` as a followup? I
> suspect we'll have more discussion around that generalization and I'd like
> for us to fix this bug because I find PR37574 somewhat concerning and I'd
> like for it to be fixed soon (like within a couple of days).
>
> > Orthogonally, the condition `__has_construct > _SourceTp const&>` is wrong because it has an extra const. It is
> > conceivable — though of course implausible/pathological — for
> > `construct(T*, T&)` to exist and do something different from `construct(T*,
> > const T&)`.
>
>
> Good catch. IIUC, `__has_construct`
> would work?
>
> Would you be OK with us applying this fix and then generalizing
> `__is_default_allocator` into `__has_trivial_construct` as a followup?
Yes, I would; although I am somewhat cynical about the followup ever actually
happening. :P
> IIUC, `__has_construct` would work?
Yes.
Comment at: libcxx/include/memory:1658
+|| !__has_construct::value) &&
+ is_trivially_move_constructible<_DestTp>::value,
void
Shouldn't this be something like `is_trivially_constructible<_DestTp,
_SourceTp&>`? I mean, we're never doing move-construction here. We're
constructing `_DestTp` from `_SourceTp&`, which is either copy-construction or
non-const-copy-construction, depending on the constness of `_SourceTp`.
`is_trivially_constructible` has pitfalls in general —
https://quuxplusone.github.io/blog/2018/07/03/trivially-constructible-from/ —
but I think we won't fall into any of those pits as long as we're testing that
`_SourceTp` is cv-qualified `_DestTp`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
ldionne added inline comments.
Comment at: libcxx/include/memory:1645
-template
+template
_LIBCPP_INLINE_VISIBILITY
Quuxplusone wrote:
> ldionne wrote:
> > Coming at it from a slightly different angle, I would think this is what we
> > want:
> >
> > ```
> > template > class _RawSourceTp = typename remove_const<_SourceTp>::type,
> > class _RawDestTp = typename remove_const<_DestTp>::type>
> > _LIBCPP_INLINE_VISIBILITY static typename enable_if<
> >
> > // We can use memcpy instead of a loop with construct if...
> > is_trivially_move_constructible<_DestTp>::value &&
> > // - the Dest is trivially move constructible, and
> > is_same<_RawSourceTp, _RawDestTp>::value &&
> > // - both types are the same modulo constness, and either
> > (__is_default_allocator::value ||
> > // + the allocator is the default allocator (and we know `construct` is
> > just placement-new), or
> > !__has_construct::value),
> > // + the allocator does not provide a custom `construct` method (so we'd
> > fall back to placement-new)
> > void>::type
> > __construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp*
> > __end1, _DestTp*& __begin2)
> > {
> > ptrdiff_t _Np = __end1 - __begin1;
> > if (_Np > 0)
> > {
> > _VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np *
> > sizeof(_DestTp));
> > __begin2 += _Np;
> > }
> > }
> > ```
> >
> > And then we should have
> >
> > ```
> > template
> > struct __is_default_allocator : false_type { };
> >
> > template
> > struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type { };
> > ```
> >
> > Does this make sense?
> >
> > Also, I'm not sure I understand why we use `const_cast` on the destination
> > type. It seems like we should instead enforce that it is non-const? But
> > this is a pre-existing thing in the code, this doesn't affect this review.
> >
> I agree that it is wrong to express the check in terms of
> `is_same>`; it should be expressed in terms of
> a trait which is satisfied by `std::allocator`-for-any-T. @ldionne
> expressed it in terms of `__is_default_allocator`. I continue to ask that
> it be expressed in terms of `__has_trivial_construct _SourceTp&>`, where libc++ specializes
> `__has_trivial_construct, ...>` if need be.
>
> Orthogonally, the condition `__has_construct _SourceTp const&>` is wrong because it has an extra `const`. It is
> conceivable — though of course implausible/pathological — for `construct(T*,
> T&)` to exist and do something different from `construct(T*, const T&)`.
> I continue to ask that it be expressed in terms of
> `__has_trivial_construct`, where libc++ specializes
> `__has_trivial_construct, ...>` if need be.
Would you be OK with us applying this fix and then generalizing
`__is_default_allocator` into `__has_trivial_construct` as a followup? I
suspect we'll have more discussion around that generalization and I'd like for
us to fix this bug because I find PR37574 somewhat concerning and I'd like for
it to be fixed soon (like within a couple of days).
> Orthogonally, the condition `__has_construct _SourceTp const&>` is wrong because it has an extra const. It is conceivable
> — though of course implausible/pathological — for `construct(T*, T&)` to
> exist and do something different from `construct(T*, const T&)`.
Good catch. IIUC, `__has_construct` would
work?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
Quuxplusone added inline comments.
Comment at: libcxx/include/memory:1645
-template
+template
_LIBCPP_INLINE_VISIBILITY
ldionne wrote:
> Coming at it from a slightly different angle, I would think this is what we
> want:
>
> ```
> templateclass _RawSourceTp = typename remove_const<_SourceTp>::type,
> class _RawDestTp = typename remove_const<_DestTp>::type>
> _LIBCPP_INLINE_VISIBILITY static typename enable_if<
>//
> We can use memcpy instead of a loop with construct if...
> is_trivially_move_constructible<_DestTp>::value && //
> - the Dest is trivially move constructible, and
> is_same<_RawSourceTp, _RawDestTp>::value &&//
> - both types are the same modulo constness, and either
> (__is_default_allocator::value || //
> + the allocator is the default allocator (and we know `construct` is just
> placement-new), or
> !__has_construct::value), //
> + the allocator does not provide a custom `construct` method (so we'd fall
> back to placement-new)
> void>::type
> __construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp*
> __end1, _DestTp*& __begin2)
> {
> ptrdiff_t _Np = __end1 - __begin1;
> if (_Np > 0)
> {
> _VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np *
> sizeof(_DestTp));
> __begin2 += _Np;
> }
> }
> ```
>
> And then we should have
>
> ```
> template
> struct __is_default_allocator : false_type { };
>
> template
> struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type { };
> ```
>
> Does this make sense?
>
> Also, I'm not sure I understand why we use `const_cast` on the destination
> type. It seems like we should instead enforce that it is non-const? But this
> is a pre-existing thing in the code, this doesn't affect this review.
>
I agree that it is wrong to express the check in terms of
`is_same>`; it should be expressed in terms of a
trait which is satisfied by `std::allocator`-for-any-T. @ldionne expressed
it in terms of `__is_default_allocator`. I continue to ask that it be
expressed in terms of `__has_trivial_construct`, where
libc++ specializes `__has_trivial_construct, ...>` if need
be.
Orthogonally, the condition `__has_construct` is wrong because it has an extra `const`. It is conceivable
— though of course implausible/pathological — for `construct(T*, T&)` to exist
and do something different from `construct(T*, const T&)`.
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:196
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
I suggest that interesting test cases include "array of `int` to vector of
`unsigned int`" (trivial, but unimplemented in this patch) and "array of
`iostream*` to vector of `ostream*`" (non-trivial because each pointer must be
adjusted).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
scanon added inline comments. Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:187 + assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON); + assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON); + assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON); These comparisons with `FLT_EPSILON` are doing nothing but adding noise. There is no value other than 2 that can possibly satisfy `fabs(v[2] - 2.0f) < FLT_EPSILON`, so this test can simply be `v[2] == 2`, for example. If you find yourself using `FLT_EPSILON` as a tolerance, it's almost always wrong. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D48342/new/ https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
ldionne added a comment.
If you use some of my suggestions, please make sure it compiles in C++03 mode.
I might be using some C++11 features (not sure whether default argument for
template parameters is C++03).
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:186
+ std::vector v(array, array + 3);
+ assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+ assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
vsapsai wrote:
> ldionne wrote:
> > I do not understand this test, can you please explain?
> At some point I had implementation that was using memcpy for initialization
> in this case. But in memory `{0, 1, 2} != {0.0f, 1.0f, 2.0f}`. I think I'll
> change the test to initialize `vector` with `float[]` and it will
> simplify asserts. Because currently those fabs and FLT_EPSILON are noisy and
> add unnecessary cognitive load.
Ok.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added a comment.
Thanks for the feedback. Answered one simple question, the rest needs more time.
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:186
+ std::vector v(array, array + 3);
+ assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+ assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
ldionne wrote:
> I do not understand this test, can you please explain?
At some point I had implementation that was using memcpy for initialization in
this case. But in memory `{0, 1, 2} != {0.0f, 1.0f, 2.0f}`. I think I'll change
the test to initialize `vector` with `float[]` and it will simplify
asserts. Because currently those fabs and FLT_EPSILON are noisy and add
unnecessary cognitive load.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: jkorous.
I believe this patch fixes an important QOI bug: see http://llvm.org/PR37574. I
wholeheartedly agree with Eric that allocators-over-const are an abomination,
however pointers-to-const are a fine thing and our implementation should handle
them gracefully.
Please rebase on top of `master` -- there's another function that does not
appear in this diff that should be fixed too (`__construct_forward`).
Comment at: libcxx/include/memory:1645
-template
+template
_LIBCPP_INLINE_VISIBILITY
Coming at it from a slightly different angle, I would think this is what we
want:
```
template ::type,
class _RawDestTp = typename remove_const<_DestTp>::type>
_LIBCPP_INLINE_VISIBILITY static typename enable_if<
//
We can use memcpy instead of a loop with construct if...
is_trivially_move_constructible<_DestTp>::value && // -
the Dest is trivially move constructible, and
is_same<_RawSourceTp, _RawDestTp>::value &&// -
both types are the same modulo constness, and either
(__is_default_allocator::value || //
+ the allocator is the default allocator (and we know `construct` is just
placement-new), or
!__has_construct::value), //
+ the allocator does not provide a custom `construct` method (so we'd fall back
to placement-new)
void>::type
__construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp*
__end1, _DestTp*& __begin2)
{
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
_VSTD::memcpy(const_cast<_RawDestTp*>(__begin2), __begin1, _Np *
sizeof(_DestTp));
__begin2 += _Np;
}
}
```
And then we should have
```
template
struct __is_default_allocator : false_type { };
template
struct __is_default_allocator<_VSTD::allocator<_Tp> > : true_type { };
```
Does this make sense?
Also, I'm not sure I understand why we use `const_cast` on the destination
type. It seems like we should instead enforce that it is non-const? But this is
a pre-existing thing in the code, this doesn't affect this review.
Comment at: libcxx/include/memory:1690
<
(is_same >::value
|| !__has_construct::value) &&
This should be fixed in a similar way.
Comment at:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:186
+ std::vector v(array, array + 3);
+ assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+ assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
I do not understand this test, can you please explain?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48342/new/
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai updated this revision to Diff 154327.
vsapsai added a comment.
- Clean up tests according to review. We don't need a new test for custom
allocators, parent patch covers that.
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
Index:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
===
---
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
+++
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
@@ -13,6 +13,8 @@
#include
#include
+#include
+#include
#include
#include "test_macros.h"
@@ -176,9 +178,20 @@
#endif
}
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+void test_ctor_with_different_value_type() {
+ int array[3] = {0, 1, 2};
+ std::vector v(array, array + 3);
+ assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+ assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+ assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);
+}
+
int main() {
basic_test_cases();
emplaceable_concept_tests(); // See PR34898
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1642,23 +1642,29 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+is_same
+<
+typename _VSTD::remove_const<_SourceTp>::type,
+typename _VSTD::remove_const<_DestTp>::type
+>::value &&
+(is_same::type> >::value
+|| is_same >::value
+|| !__has_construct::value) &&
+ is_trivially_move_constructible<_DestTp>::value,
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1,
_Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1,
_SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
+typedef typename remove_const<_DestTp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np *
sizeof(_Tp));
+_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np *
sizeof(_DestTp));
__begin2 += _Np;
}
}
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
===
--- libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp
@@ -13,6 +13,8 @@
#include
#include
+#include
+#include
#include
#include "test_macros.h"
@@ -176,9 +178,20 @@
#endif
}
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+void test_ctor_with_different_value_type() {
+ int array[3] = {0, 1, 2};
+ std::vector v(array, array + 3);
+ assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+ assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+ assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);
+}
+
int main() {
basic_test_cases();
emplaceable_concept_tests(); // See PR34898
test_ctor_under_alloc();
+ test_ctor_with_different_value_type();
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1642,23 +1642,29 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+is_same
+<
+typename _VSTD::remove_const<_SourceTp>::type,
+typename _VSTD::remove_const<_DestTp>::type
+>::value &&
+(is_same::type> >::value
+|| is_same >::value
+|| !__has_construct::value) &&
+ is_trivially_move_constructible<_DestTp>::value,
void
>::type
-
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
Quuxplusone wrote:
> vsapsai wrote:
> > Quuxplusone wrote:
> > > vsapsai wrote:
> > > > vsapsai wrote:
> > > > > vsapsai wrote:
> > > > > > erik.pilkington wrote:
> > > > > > > vsapsai wrote:
> > > > > > > > erik.pilkington wrote:
> > > > > > > > > Shouldn't this be true_type?
> > > > > > > > I see this is confusing and I'm still struggling how to express
> > > > > > > > it. The issue is that in C++03 `__has_construct` should be
> > > > > > > > something like unknown, so that neither `__has_construct` nor
> > > > > > > > `! __has_construct` evaluate to true because we don't really
> > > > > > > > know if allocator has construct. This case is covered by the
> > > > > > > > added test, in C++03 the memcpy specialization was enabled when
> > > > > > > >
> > > > > > > > ```
> > > > > > > > is_same >
> > > > > > > > || !false_type
> > > > > > > > ```
> > > > > > > >
> > > > > > > > So `is_same` check had no effect and we were using memcpy to
> > > > > > > > convert between int and float.
> > > > > > > >
> > > > > > > > I was considering using something like
> > > > > > > >
> > > > > > > > ```lang=c++
> > > > > > > > typename enable_if
> > > > > > > > <
> > > > > > > > (is_same
> > > > > > > > <
> > > > > > > > typename _VSTD::remove_const > > > > > > > allocator_type::value_type>::type,
> > > > > > > > typename _VSTD::remove_const<_SourceTp>::type
> > > > > > > > >::value
> > > > > > > > #ifndef _LIBCPP_CXX03_LANG
> > > > > > > > || !__has_construct > > > > > > > _SourceTp>::value
> > > > > > > > #endif
> > > > > > > > ) &&
> > > > > > > > is_trivially_move_constructible<_DestTp>::value,
> > > > > > > > void
> > > > > > > > >::type
> > > > > > > > ```
> > > > > > > >
> > > > > > > > But that doesn't look readable to me, so I've introduced ad-hoc
> > > > > > > > ternary logic with `__has_construct_missing`.
> > > > > > > Oh I see, yikes! That's a pretty bad bug. I agree that this fix
> > > > > > > is best then, but can you add a comment explaining this to
> > > > > > > `__has_construct_missing` for future casual readers? Also, I
> > > > > > > think we should move the __has_construct_missing bugfix into a
> > > > > > > different (prerequisite) patch. Seems unrelated to the `const`
> > > > > > > related optimization below.
> > > > > > The bug as I described isn't really present now because function
> > > > > > signature
> > > > > >
> > > > > > __construct_range_forward(allocator_type&, _Tp* __begin1, _Tp*
> > > > > > __end1, _Tp*& __begin2)
> > > > > >
> > > > > > works as implicit `is_same` for `__begin1` and `__begin2` types. I
> > > > > > think it is worth fixing separately and there is a bug with C++03
> > > > > > and custom allocators.
> > > > > Instead of `__has_construct_missing` I've implemented real
> > > > > `__has_construct` in D48753. But it is stricter in C++03 than in
> > > > > C++11 and later. So it made me think that absence of `construct` with
> > > > > exact signature isn't a good reason to use memcpy.
> > > > I was wrong. Now I think the logic for using memcpy should be
> > > >
> > > > if types are the same modulo constness
> > > > and
> > > > (
> > > > using default allocator
> > > > or
> > > > using custom allocator without `construct`
> > > > )
> > > > and
> > > > is_trivially_move_constructible
> > > >
> > > > The purpose of the allocator check is to cover cases when `static
> > > > construct` would end up calling not user's code but libc++ code that we
> > > > know can be replaced with memcpy.
> > > I'd like to request the spelling `__has_trivial_construct_and_destroy > > T, T&&>` as described here:
> > > https://www.youtube.com/watch?v=MWBfmmg8-Yo=21m45s
> > > I have an implementation in my fork that might be useful for comparison,
> > > although even it doesn't add that last `T&&` parameter.
> > > https://github.com/Quuxplusone/libcxx/commit/34eb0b5c8f03880b94d53b877cbca384783ad65a
> > >
> > > What we're *really* interested in, in most cases, is
> > > `__has_trivial_construct && __has_trivial_destroy`. We
> > > don't care if there happens to exist an obscure overload such as
> > > `A::construct(T*, Widget, Gadget)` as long as it is not selected. This is
> > > particularly relevant to `scoped_allocator_adaptor`, whose `construct` is
> > > trivial for primitive types but non-trivial for allocator-aware types.
> > >
> > > Also, when we write out the template type parameters we care about, we
> > > can do the decltype stuff really easily, without having to "fudge" the
> > > metaprogramming and possibly get it wrong. For example, if `A::construct`
> > > is an overload set, in which case the
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added a comment. In https://reviews.llvm.org/D48342#1152063, @EricWF wrote: > Are there any tests which actually exercise the new behavior? Added tests only verify we don't use memcpy erroneously. And existing tests make sure there are no functionality regressions. But there is nothing to test the performance improvement. Are there any recommendations for that? Comment at: libcxx/include/memory:1665 +(is_same::type> >::value +|| is_same >::value +|| !__has_construct::value) && EricWF wrote: > I'm not sure we should care about allocators for `T const`. The're all but an > abomination. My main goal was to avoid performance regression for std::vector v(const_raw, const_raw + SIZE); I'm not protecting allocators for `T const`, it just seems cheap to support them anyway. Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp:13 +// template vector(InputIter first, InputIter last); + +// Initialize a vector with a different value type. Make sure initialization EricWF wrote: > Can this be folded into an existing test file for the constructor it's > targeting? Will move to construct_iter_iter.pass.cpp. Which reminded me that I need to make construct_iter_iter_alloc.pass.cpp and construct_iter_iter.pass.cpp more in sync. https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
EricWF added a comment. Are there any tests which actually exercise the new behavior? Comment at: libcxx/include/memory:1665 +(is_same::type> >::value +|| is_same >::value +|| !__has_construct::value) && I'm not sure we should care about allocators for `T const`. The're all but an abomination. Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp:13 +// template vector(InputIter first, InputIter last); + +// Initialize a vector with a different value type. Make sure initialization Can this be folded into an existing test file for the constructor it's targeting? https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
Quuxplusone added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
vsapsai wrote:
> Quuxplusone wrote:
> > vsapsai wrote:
> > > vsapsai wrote:
> > > > vsapsai wrote:
> > > > > erik.pilkington wrote:
> > > > > > vsapsai wrote:
> > > > > > > erik.pilkington wrote:
> > > > > > > > Shouldn't this be true_type?
> > > > > > > I see this is confusing and I'm still struggling how to express
> > > > > > > it. The issue is that in C++03 `__has_construct` should be
> > > > > > > something like unknown, so that neither `__has_construct` nor `!
> > > > > > > __has_construct` evaluate to true because we don't really know if
> > > > > > > allocator has construct. This case is covered by the added test,
> > > > > > > in C++03 the memcpy specialization was enabled when
> > > > > > >
> > > > > > > ```
> > > > > > > is_same >
> > > > > > > || !false_type
> > > > > > > ```
> > > > > > >
> > > > > > > So `is_same` check had no effect and we were using memcpy to
> > > > > > > convert between int and float.
> > > > > > >
> > > > > > > I was considering using something like
> > > > > > >
> > > > > > > ```lang=c++
> > > > > > > typename enable_if
> > > > > > > <
> > > > > > > (is_same
> > > > > > > <
> > > > > > > typename _VSTD::remove_const > > > > > > allocator_type::value_type>::type,
> > > > > > > typename _VSTD::remove_const<_SourceTp>::type
> > > > > > > >::value
> > > > > > > #ifndef _LIBCPP_CXX03_LANG
> > > > > > > || !__has_construct > > > > > > _SourceTp>::value
> > > > > > > #endif
> > > > > > > ) &&
> > > > > > > is_trivially_move_constructible<_DestTp>::value,
> > > > > > > void
> > > > > > > >::type
> > > > > > > ```
> > > > > > >
> > > > > > > But that doesn't look readable to me, so I've introduced ad-hoc
> > > > > > > ternary logic with `__has_construct_missing`.
> > > > > > Oh I see, yikes! That's a pretty bad bug. I agree that this fix is
> > > > > > best then, but can you add a comment explaining this to
> > > > > > `__has_construct_missing` for future casual readers? Also, I think
> > > > > > we should move the __has_construct_missing bugfix into a different
> > > > > > (prerequisite) patch. Seems unrelated to the `const` related
> > > > > > optimization below.
> > > > > The bug as I described isn't really present now because function
> > > > > signature
> > > > >
> > > > > __construct_range_forward(allocator_type&, _Tp* __begin1, _Tp*
> > > > > __end1, _Tp*& __begin2)
> > > > >
> > > > > works as implicit `is_same` for `__begin1` and `__begin2` types. I
> > > > > think it is worth fixing separately and there is a bug with C++03 and
> > > > > custom allocators.
> > > > Instead of `__has_construct_missing` I've implemented real
> > > > `__has_construct` in D48753. But it is stricter in C++03 than in C++11
> > > > and later. So it made me think that absence of `construct` with exact
> > > > signature isn't a good reason to use memcpy.
> > > I was wrong. Now I think the logic for using memcpy should be
> > >
> > > if types are the same modulo constness
> > > and
> > > (
> > > using default allocator
> > > or
> > > using custom allocator without `construct`
> > > )
> > > and
> > > is_trivially_move_constructible
> > >
> > > The purpose of the allocator check is to cover cases when `static
> > > construct` would end up calling not user's code but libc++ code that we
> > > know can be replaced with memcpy.
> > I'd like to request the spelling `__has_trivial_construct_and_destroy > T&&>` as described here:
> > https://www.youtube.com/watch?v=MWBfmmg8-Yo=21m45s
> > I have an implementation in my fork that might be useful for comparison,
> > although even it doesn't add that last `T&&` parameter.
> > https://github.com/Quuxplusone/libcxx/commit/34eb0b5c8f03880b94d53b877cbca384783ad65a
> >
> > What we're *really* interested in, in most cases, is
> > `__has_trivial_construct && __has_trivial_destroy`. We
> > don't care if there happens to exist an obscure overload such as
> > `A::construct(T*, Widget, Gadget)` as long as it is not selected. This is
> > particularly relevant to `scoped_allocator_adaptor`, whose `construct` is
> > trivial for primitive types but non-trivial for allocator-aware types.
> >
> > Also, when we write out the template type parameters we care about, we can
> > do the decltype stuff really easily, without having to "fudge" the
> > metaprogramming and possibly get it wrong. For example, if `A::construct`
> > is an overload set, in which case the `&_Xp::construct` on this patch's
> > line 1492 will be ill-formed even though a non-trivial `construct` does
> > actually exist.
> I agree with benefits of using `__has_trivial_construct_and_destroy` in
>
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
Quuxplusone wrote:
> vsapsai wrote:
> > vsapsai wrote:
> > > vsapsai wrote:
> > > > erik.pilkington wrote:
> > > > > vsapsai wrote:
> > > > > > erik.pilkington wrote:
> > > > > > > Shouldn't this be true_type?
> > > > > > I see this is confusing and I'm still struggling how to express it.
> > > > > > The issue is that in C++03 `__has_construct` should be something
> > > > > > like unknown, so that neither `__has_construct` nor `!
> > > > > > __has_construct` evaluate to true because we don't really know if
> > > > > > allocator has construct. This case is covered by the added test, in
> > > > > > C++03 the memcpy specialization was enabled when
> > > > > >
> > > > > > ```
> > > > > > is_same >
> > > > > > || !false_type
> > > > > > ```
> > > > > >
> > > > > > So `is_same` check had no effect and we were using memcpy to
> > > > > > convert between int and float.
> > > > > >
> > > > > > I was considering using something like
> > > > > >
> > > > > > ```lang=c++
> > > > > > typename enable_if
> > > > > > <
> > > > > > (is_same
> > > > > > <
> > > > > > typename _VSTD::remove_const > > > > > allocator_type::value_type>::type,
> > > > > > typename _VSTD::remove_const<_SourceTp>::type
> > > > > > >::value
> > > > > > #ifndef _LIBCPP_CXX03_LANG
> > > > > > || !__has_construct > > > > > _SourceTp>::value
> > > > > > #endif
> > > > > > ) &&
> > > > > > is_trivially_move_constructible<_DestTp>::value,
> > > > > > void
> > > > > > >::type
> > > > > > ```
> > > > > >
> > > > > > But that doesn't look readable to me, so I've introduced ad-hoc
> > > > > > ternary logic with `__has_construct_missing`.
> > > > > Oh I see, yikes! That's a pretty bad bug. I agree that this fix is
> > > > > best then, but can you add a comment explaining this to
> > > > > `__has_construct_missing` for future casual readers? Also, I think we
> > > > > should move the __has_construct_missing bugfix into a different
> > > > > (prerequisite) patch. Seems unrelated to the `const` related
> > > > > optimization below.
> > > > The bug as I described isn't really present now because function
> > > > signature
> > > >
> > > > __construct_range_forward(allocator_type&, _Tp* __begin1, _Tp*
> > > > __end1, _Tp*& __begin2)
> > > >
> > > > works as implicit `is_same` for `__begin1` and `__begin2` types. I
> > > > think it is worth fixing separately and there is a bug with C++03 and
> > > > custom allocators.
> > > Instead of `__has_construct_missing` I've implemented real
> > > `__has_construct` in D48753. But it is stricter in C++03 than in C++11
> > > and later. So it made me think that absence of `construct` with exact
> > > signature isn't a good reason to use memcpy.
> > I was wrong. Now I think the logic for using memcpy should be
> >
> > if types are the same modulo constness
> > and
> > (
> > using default allocator
> > or
> > using custom allocator without `construct`
> > )
> > and
> > is_trivially_move_constructible
> >
> > The purpose of the allocator check is to cover cases when `static
> > construct` would end up calling not user's code but libc++ code that we
> > know can be replaced with memcpy.
> I'd like to request the spelling `__has_trivial_construct_and_destroy T&&>` as described here: https://www.youtube.com/watch?v=MWBfmmg8-Yo=21m45s
> I have an implementation in my fork that might be useful for comparison,
> although even it doesn't add that last `T&&` parameter.
> https://github.com/Quuxplusone/libcxx/commit/34eb0b5c8f03880b94d53b877cbca384783ad65a
>
> What we're *really* interested in, in most cases, is
> `__has_trivial_construct && __has_trivial_destroy`. We don't
> care if there happens to exist an obscure overload such as `A::construct(T*,
> Widget, Gadget)` as long as it is not selected. This is particularly relevant
> to `scoped_allocator_adaptor`, whose `construct` is trivial for primitive
> types but non-trivial for allocator-aware types.
>
> Also, when we write out the template type parameters we care about, we can do
> the decltype stuff really easily, without having to "fudge" the
> metaprogramming and possibly get it wrong. For example, if `A::construct` is
> an overload set, in which case the `&_Xp::construct` on this patch's line
> 1492 will be ill-formed even though a non-trivial `construct` does actually
> exist.
I agree with benefits of using `__has_trivial_construct_and_destroy` in general
but in this case I don't see a need for `__has_trivial_destroy`.
`__construct_range_forward` only copies new elements and it isn't concerned
with destruction. Probably for some cases we can meld destroy+construct
together
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai updated this revision to Diff 154021.
vsapsai added a comment.
- Proper support for custom allocators without `construct`.
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
Index:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
===
--- /dev/null
+++
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+//
+
+// template vector(InputIter first, InputIter last);
+
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+
+#include
+#include
+#include
+#include
+
+int main()
+{
+int array[3] = {0, 1, 2};
+std::vector v(array, array + 3);
+assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);
+}
Index:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
===
---
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
+++
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
@@ -195,6 +195,17 @@
//C v(It(arr2), It(std::end(arr2)), a);
}
}
+ {
+using C = std::vector >;
+{
+ ExpectConstructGuard G(1);
+ C v(arr1, arr1 + 1);
+}
+{
+ ExpectConstructGuard G(3);
+ C v(arr2, arr2 + 3);
+}
+ }
#endif
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1651,23 +1651,29 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+is_same
+<
+typename _VSTD::remove_const<_SourceTp>::type,
+typename _VSTD::remove_const<_DestTp>::type
+>::value &&
+(is_same::type> >::value
+|| is_same >::value
+|| !__has_construct::value) &&
+ is_trivially_move_constructible<_DestTp>::value,
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1,
_Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1,
_SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
+typedef typename remove_const<_DestTp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np *
sizeof(_Tp));
+_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np *
sizeof(_DestTp));
__begin2 += _Np;
}
}
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+//
+
+// template vector(InputIter first, InputIter last);
+
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+
+#include
+#include
+#include
+#include
+
+int main()
+{
+int array[3] = {0, 1, 2};
+std::vector v(array, array + 3);
+assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+assert(std::fabs(v[2]
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
Quuxplusone added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
vsapsai wrote:
> vsapsai wrote:
> > vsapsai wrote:
> > > erik.pilkington wrote:
> > > > vsapsai wrote:
> > > > > erik.pilkington wrote:
> > > > > > Shouldn't this be true_type?
> > > > > I see this is confusing and I'm still struggling how to express it.
> > > > > The issue is that in C++03 `__has_construct` should be something like
> > > > > unknown, so that neither `__has_construct` nor `! __has_construct`
> > > > > evaluate to true because we don't really know if allocator has
> > > > > construct. This case is covered by the added test, in C++03 the
> > > > > memcpy specialization was enabled when
> > > > >
> > > > > ```
> > > > > is_same >
> > > > > || !false_type
> > > > > ```
> > > > >
> > > > > So `is_same` check had no effect and we were using memcpy to convert
> > > > > between int and float.
> > > > >
> > > > > I was considering using something like
> > > > >
> > > > > ```lang=c++
> > > > > typename enable_if
> > > > > <
> > > > > (is_same
> > > > > <
> > > > > typename _VSTD::remove_const > > > > allocator_type::value_type>::type,
> > > > > typename _VSTD::remove_const<_SourceTp>::type
> > > > > >::value
> > > > > #ifndef _LIBCPP_CXX03_LANG
> > > > > || !__has_construct > > > > _SourceTp>::value
> > > > > #endif
> > > > > ) &&
> > > > > is_trivially_move_constructible<_DestTp>::value,
> > > > > void
> > > > > >::type
> > > > > ```
> > > > >
> > > > > But that doesn't look readable to me, so I've introduced ad-hoc
> > > > > ternary logic with `__has_construct_missing`.
> > > > Oh I see, yikes! That's a pretty bad bug. I agree that this fix is best
> > > > then, but can you add a comment explaining this to
> > > > `__has_construct_missing` for future casual readers? Also, I think we
> > > > should move the __has_construct_missing bugfix into a different
> > > > (prerequisite) patch. Seems unrelated to the `const` related
> > > > optimization below.
> > > The bug as I described isn't really present now because function signature
> > >
> > > __construct_range_forward(allocator_type&, _Tp* __begin1, _Tp*
> > > __end1, _Tp*& __begin2)
> > >
> > > works as implicit `is_same` for `__begin1` and `__begin2` types. I think
> > > it is worth fixing separately and there is a bug with C++03 and custom
> > > allocators.
> > Instead of `__has_construct_missing` I've implemented real
> > `__has_construct` in D48753. But it is stricter in C++03 than in C++11 and
> > later. So it made me think that absence of `construct` with exact signature
> > isn't a good reason to use memcpy.
> I was wrong. Now I think the logic for using memcpy should be
>
> if types are the same modulo constness
> and
> (
> using default allocator
> or
> using custom allocator without `construct`
> )
> and
> is_trivially_move_constructible
>
> The purpose of the allocator check is to cover cases when `static construct`
> would end up calling not user's code but libc++ code that we know can be
> replaced with memcpy.
I'd like to request the spelling `__has_trivial_construct_and_destroy` as described here: https://www.youtube.com/watch?v=MWBfmmg8-Yo=21m45s
I have an implementation in my fork that might be useful for comparison,
although even it doesn't add that last `T&&` parameter.
https://github.com/Quuxplusone/libcxx/commit/34eb0b5c8f03880b94d53b877cbca384783ad65a
What we're *really* interested in, in most cases, is
`__has_trivial_construct && __has_trivial_destroy`. We don't
care if there happens to exist an obscure overload such as `A::construct(T*,
Widget, Gadget)` as long as it is not selected. This is particularly relevant
to `scoped_allocator_adaptor`, whose `construct` is trivial for primitive types
but non-trivial for allocator-aware types.
Also, when we write out the template type parameters we care about, we can do
the decltype stuff really easily, without having to "fudge" the metaprogramming
and possibly get it wrong. For example, if `A::construct` is an overload set,
in which case the `&_Xp::construct` on this patch's line 1492 will be
ill-formed even though a non-trivial `construct` does actually exist.
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai planned changes to this revision.
vsapsai added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
vsapsai wrote:
> vsapsai wrote:
> > erik.pilkington wrote:
> > > vsapsai wrote:
> > > > erik.pilkington wrote:
> > > > > Shouldn't this be true_type?
> > > > I see this is confusing and I'm still struggling how to express it. The
> > > > issue is that in C++03 `__has_construct` should be something like
> > > > unknown, so that neither `__has_construct` nor `! __has_construct`
> > > > evaluate to true because we don't really know if allocator has
> > > > construct. This case is covered by the added test, in C++03 the memcpy
> > > > specialization was enabled when
> > > >
> > > > ```
> > > > is_same >
> > > > || !false_type
> > > > ```
> > > >
> > > > So `is_same` check had no effect and we were using memcpy to convert
> > > > between int and float.
> > > >
> > > > I was considering using something like
> > > >
> > > > ```lang=c++
> > > > typename enable_if
> > > > <
> > > > (is_same
> > > > <
> > > > typename _VSTD::remove_const > > > allocator_type::value_type>::type,
> > > > typename _VSTD::remove_const<_SourceTp>::type
> > > > >::value
> > > > #ifndef _LIBCPP_CXX03_LANG
> > > > || !__has_construct > > > _SourceTp>::value
> > > > #endif
> > > > ) &&
> > > > is_trivially_move_constructible<_DestTp>::value,
> > > > void
> > > > >::type
> > > > ```
> > > >
> > > > But that doesn't look readable to me, so I've introduced ad-hoc ternary
> > > > logic with `__has_construct_missing`.
> > > Oh I see, yikes! That's a pretty bad bug. I agree that this fix is best
> > > then, but can you add a comment explaining this to
> > > `__has_construct_missing` for future casual readers? Also, I think we
> > > should move the __has_construct_missing bugfix into a different
> > > (prerequisite) patch. Seems unrelated to the `const` related optimization
> > > below.
> > The bug as I described isn't really present now because function signature
> >
> > __construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1,
> > _Tp*& __begin2)
> >
> > works as implicit `is_same` for `__begin1` and `__begin2` types. I think it
> > is worth fixing separately and there is a bug with C++03 and custom
> > allocators.
> Instead of `__has_construct_missing` I've implemented real `__has_construct`
> in D48753. But it is stricter in C++03 than in C++11 and later. So it made me
> think that absence of `construct` with exact signature isn't a good reason to
> use memcpy.
I was wrong. Now I think the logic for using memcpy should be
if types are the same modulo constness
and
(
using default allocator
or
using custom allocator without `construct`
)
and
is_trivially_move_constructible
The purpose of the allocator check is to cover cases when `static construct`
would end up calling not user's code but libc++ code that we know can be
replaced with memcpy.
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added a comment. In https://reviews.llvm.org/D48342#1148751, @mclow.lists wrote: > I want to point out that this code (not -necessarily- this patch, but where > it lives) needs to be rewritten. > > There is no prohibition on users specializing `allocator_traits` for their > allocators, and yet libc++'s vector depends on the existence of > `allocator_traits::__construct_range_forward` (among other things). Marshall, do you agree to have such rewrite to be done separately? It seems to me fairly involved and I don't want it to block other changes. And for this change I suggest observing how it develops. If it is fairly small and isolated, I think it would be useful to have it before the more substantial `allocator_traits` rewrite. https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
mclow.lists added a comment. I want to point out that this code (not -necessarily- this patch, but where it lives) needs to be rewritten. There is no prohibition on users specializing `allocator_traits` for their allocators, and yet libc++'s vector depends on the existence of `allocator_traits::__construct_range_forward` (among other things). https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
vsapsai wrote:
> erik.pilkington wrote:
> > vsapsai wrote:
> > > erik.pilkington wrote:
> > > > Shouldn't this be true_type?
> > > I see this is confusing and I'm still struggling how to express it. The
> > > issue is that in C++03 `__has_construct` should be something like
> > > unknown, so that neither `__has_construct` nor `! __has_construct`
> > > evaluate to true because we don't really know if allocator has construct.
> > > This case is covered by the added test, in C++03 the memcpy
> > > specialization was enabled when
> > >
> > > ```
> > > is_same >
> > > || !false_type
> > > ```
> > >
> > > So `is_same` check had no effect and we were using memcpy to convert
> > > between int and float.
> > >
> > > I was considering using something like
> > >
> > > ```lang=c++
> > > typename enable_if
> > > <
> > > (is_same
> > > <
> > > typename _VSTD::remove_const > > allocator_type::value_type>::type,
> > > typename _VSTD::remove_const<_SourceTp>::type
> > > >::value
> > > #ifndef _LIBCPP_CXX03_LANG
> > > || !__has_construct > > _SourceTp>::value
> > > #endif
> > > ) &&
> > > is_trivially_move_constructible<_DestTp>::value,
> > > void
> > > >::type
> > > ```
> > >
> > > But that doesn't look readable to me, so I've introduced ad-hoc ternary
> > > logic with `__has_construct_missing`.
> > Oh I see, yikes! That's a pretty bad bug. I agree that this fix is best
> > then, but can you add a comment explaining this to
> > `__has_construct_missing` for future casual readers? Also, I think we
> > should move the __has_construct_missing bugfix into a different
> > (prerequisite) patch. Seems unrelated to the `const` related optimization
> > below.
> The bug as I described isn't really present now because function signature
>
> __construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1,
> _Tp*& __begin2)
>
> works as implicit `is_same` for `__begin1` and `__begin2` types. I think it
> is worth fixing separately and there is a bug with C++03 and custom
> allocators.
Instead of `__has_construct_missing` I've implemented real `__has_construct` in
D48753. But it is stricter in C++03 than in C++11 and later. So it made me
think that absence of `construct` with exact signature isn't a good reason to
use memcpy.
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai updated this revision to Diff 153431.
vsapsai added a comment.
Try to exclude changes available in parent review from this review.
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
Index:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
===
--- /dev/null
+++
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+//
+
+// template vector(InputIter first, InputIter last);
+
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+
+#include
+#include
+#include
+#include
+
+int main()
+{
+int array[3] = {0, 1, 2};
+std::vector v(array, array + 3);
+assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);
+}
Index:
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
===
---
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
+++
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
@@ -195,6 +195,17 @@
//C v(It(arr2), It(std::end(arr2)), a);
}
}
+ {
+using C = std::vector >;
+{
+ ExpectConstructGuard G(1);
+ C v(arr1, arr1 + 1);
+}
+{
+ ExpectConstructGuard G(3);
+ C v(arr2, arr2 + 3);
+}
+ }
#endif
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1728,23 +1728,23 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+(is_same::type> >::value
+|| is_same
>::value) &&
+ is_trivially_move_constructible<_DestTp>::value,
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1,
_Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1,
_SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
+typedef typename remove_const<_DestTp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np *
sizeof(_Tp));
+_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np *
sizeof(_DestTp));
__begin2 += _Np;
}
}
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+//
+
+// template vector(InputIter first, InputIter last);
+
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+
+#include
+#include
+#include
+#include
+
+int main()
+{
+int array[3] = {0, 1, 2};
+std::vector v(array, array + 3);
+assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);
+}
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
===
---
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai updated this revision to Diff 153430.
vsapsai added a comment.
Herald added a subscriber: dexonsmith.
- Don't check `!__has_construct` for `__construct_range_forward`.
Incompatible types will cause a lack of `construct` but it doesn't mean we
should use memcpy instead. And missing `_Alloc::construct` doesn't mean
`alloc_traits::construct` will fail, so falling back on memcpy can be
premature.
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+//
+
+// template vector(InputIter first, InputIter last);
+
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+
+#include
+#include
+#include
+#include
+
+int main()
+{
+int array[3] = {0, 1, 2};
+std::vector v(array, array + 3);
+assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);
+}
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
===
--- libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
@@ -50,6 +50,26 @@
#endif
+template
+struct cpp03_allocator : bare_allocator {
+ typedef T value_type;
+ typedef value_type* pointer;
+
+ static bool construct_called;
+
+ void construct(pointer p, const value_type& val)
+ {
+::new(p) value_type(val);
+construct_called = true;
+ }
+
+ std::size_t max_size() const
+ {
+return UINT_MAX / sizeof(T);
+ }
+};
+template bool cpp03_allocator::construct_called = false;
+
void basic_tests() {
{
int a[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 8, 7, 6, 5, 4, 3, 1, 0};
@@ -129,9 +149,22 @@
}
void test_ctor_under_alloc() {
-#if TEST_STD_VER >= 11
int arr1[] = {42};
int arr2[] = {1, 101, 42};
+ {
+typedef std::vector > C;
+{
+ cpp03_allocator::construct_called = false;
+ C v(arr1, arr1 + 1);
+ assert(cpp03_allocator::construct_called);
+}
+{
+ cpp03_allocator::construct_called = false;
+ C v(arr2, arr2 + 3);
+ assert(cpp03_allocator::construct_called);
+}
+ }
+#if TEST_STD_VER >= 11
{
using C = TCT::vector<>;
using T = typename C::value_type;
@@ -162,6 +195,17 @@
//C v(It(arr2), It(std::end(arr2)), a);
}
}
+ {
+using C = std::vector >;
+{
+ ExpectConstructGuard G(1);
+ C v(arr1, arr1 + 1);
+}
+{
+ ExpectConstructGuard G(3);
+ C v(arr2, arr2 + 3);
+}
+ }
#endif
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1459,24 +1459,102 @@
#else // _LIBCPP_CXX03_LANG
-#ifndef _LIBCPP_HAS_NO_VARIADICS
+template
+struct __has_construct_test_0
+{
+private:
+template static true_type __test(void (_Xp::*)(_Pointer));
+template static false_type __test(...);
-template
-struct __has_construct
-: false_type
+template
+static decltype(__test(&_Xp::construct)) __test_exist(decltype(&_Xp::construct));
+template static false_type __test_exist(...);
+
+typedef decltype(__test_exist<_Alloc>(0)) type;
+public:
+static const bool value = type::value;
+};
+
+template
+struct __has_construct_0
+: integral_constant::value>
{
};
-#else // _LIBCPP_HAS_NO_VARIADICS
+template
+struct __has_construct_test_1
+{
+private:
+template static true_type __test(void (_Xp::*)(_Pointer, _A0));
+template static false_type __test(...);
+
+template
+static decltype(__test(&_Xp::construct)) __test_exist(decltype(&_Xp::construct));
+template static false_type __test_exist(...);
+
+typedef decltype(__test_exist<_Alloc>(0)) type;
+public:
+static const bool value = type::value;
+};
+
+template
+struct __has_construct_1
+: integral_constant::value>
+{
+};
+
+template
+struct
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
erik.pilkington wrote:
> vsapsai wrote:
> > erik.pilkington wrote:
> > > Shouldn't this be true_type?
> > I see this is confusing and I'm still struggling how to express it. The
> > issue is that in C++03 `__has_construct` should be something like unknown,
> > so that neither `__has_construct` nor `! __has_construct` evaluate to true
> > because we don't really know if allocator has construct. This case is
> > covered by the added test, in C++03 the memcpy specialization was enabled
> > when
> >
> > ```
> > is_same >
> > || !false_type
> > ```
> >
> > So `is_same` check had no effect and we were using memcpy to convert
> > between int and float.
> >
> > I was considering using something like
> >
> > ```lang=c++
> > typename enable_if
> > <
> > (is_same
> > <
> > typename _VSTD::remove_const > allocator_type::value_type>::type,
> > typename _VSTD::remove_const<_SourceTp>::type
> > >::value
> > #ifndef _LIBCPP_CXX03_LANG
> > || !__has_construct > _SourceTp>::value
> > #endif
> > ) &&
> > is_trivially_move_constructible<_DestTp>::value,
> > void
> > >::type
> > ```
> >
> > But that doesn't look readable to me, so I've introduced ad-hoc ternary
> > logic with `__has_construct_missing`.
> Oh I see, yikes! That's a pretty bad bug. I agree that this fix is best then,
> but can you add a comment explaining this to `__has_construct_missing` for
> future casual readers? Also, I think we should move the
> __has_construct_missing bugfix into a different (prerequisite) patch. Seems
> unrelated to the `const` related optimization below.
The bug as I described isn't really present now because function signature
__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1,
_Tp*& __begin2)
works as implicit `is_same` for `__begin1` and `__begin2` types. I think it is
worth fixing separately and there is a bug with C++03 and custom allocators.
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
erik.pilkington added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
vsapsai wrote:
> erik.pilkington wrote:
> > Shouldn't this be true_type?
> I see this is confusing and I'm still struggling how to express it. The issue
> is that in C++03 `__has_construct` should be something like unknown, so that
> neither `__has_construct` nor `! __has_construct` evaluate to true because we
> don't really know if allocator has construct. This case is covered by the
> added test, in C++03 the memcpy specialization was enabled when
>
> ```
> is_same >
> || !false_type
> ```
>
> So `is_same` check had no effect and we were using memcpy to convert between
> int and float.
>
> I was considering using something like
>
> ```lang=c++
> typename enable_if
> <
> (is_same
> <
> typename _VSTD::remove_const allocator_type::value_type>::type,
> typename _VSTD::remove_const<_SourceTp>::type
> >::value
> #ifndef _LIBCPP_CXX03_LANG
> || !__has_construct _SourceTp>::value
> #endif
> ) &&
> is_trivially_move_constructible<_DestTp>::value,
> void
> >::type
> ```
>
> But that doesn't look readable to me, so I've introduced ad-hoc ternary logic
> with `__has_construct_missing`.
Oh I see, yikes! That's a pretty bad bug. I agree that this fix is best then,
but can you add a comment explaining this to `__has_construct_missing` for
future casual readers? Also, I think we should move the __has_construct_missing
bugfix into a different (prerequisite) patch. Seems unrelated to the `const`
related optimization below.
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai updated this revision to Diff 152990.
vsapsai added a comment.
- Don't use memcpy specialization with custom allocators.
Not entirely satisfied with comparing allocator to non-const and const, don't
know if there is a more elegant way to do the same.
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+//
+
+// template vector(InputIter first, InputIter last);
+
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+
+#include
+#include
+#include
+#include
+
+int main()
+{
+int array[3] = {0, 1, 2};
+std::vector v(array, array + 3);
+assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);
+}
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
===
--- libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp
@@ -162,6 +162,17 @@
//C v(It(arr2), It(std::end(arr2)), a);
}
}
+ {
+using C = std::vector >;
+{
+ ExpectConstructGuard G(1);
+ C v(arr1, arr1 + 1);
+}
+{
+ ExpectConstructGuard G(3);
+ C v(arr2, arr2 + 3);
+}
+ }
#endif
}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1399,6 +1399,13 @@
{
};
+template
+struct __has_construct_missing
+: integral_constant::value>
+{
+};
+
template
auto
__has_destroy_test(_Alloc&& __a, _Pointer&& __p)
@@ -1467,14 +1474,26 @@
{
};
+template
+struct __has_construct_missing
+: false_type
+{
+};
+
#else // _LIBCPP_HAS_NO_VARIADICS
template
struct __has_construct
: false_type
{
};
+template
+struct __has_construct_missing
+: false_type
+{
+};
+
#endif // _LIBCPP_HAS_NO_VARIADICS
template
@@ -1622,7 +1641,7 @@
typename enable_if
<
(is_same >::value
-|| !__has_construct::value) &&
+|| __has_construct_missing::value) &&
is_trivially_move_constructible<_Tp>::value,
void
>::type
@@ -1646,23 +1665,24 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+(is_same::type> >::value
+|| is_same >::value
+|| __has_construct_missing::value) &&
+ is_trivially_move_constructible<_DestTp>::value,
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
+typedef typename remove_const<_DestTp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np * sizeof(_Tp));
+_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np * sizeof(_DestTp));
__begin2 += _Np;
}
}
@@ -1686,7 +1706,7 @@
typename enable_if
<
(is_same >::value
-|| !__has_construct::value) &&
+|| __has_construct_missing::value) &&
is_trivially_move_constructible<_Tp>::value,
void
>::type
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai planned changes to this revision.
vsapsai added inline comments.
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
erik.pilkington wrote:
> Shouldn't this be true_type?
I see this is confusing and I'm still struggling how to express it. The issue
is that in C++03 `__has_construct` should be something like unknown, so that
neither `__has_construct` nor `! __has_construct` evaluate to true because we
don't really know if allocator has construct. This case is covered by the added
test, in C++03 the memcpy specialization was enabled when
```
is_same >
|| !false_type
```
So `is_same` check had no effect and we were using memcpy to convert between
int and float.
I was considering using something like
```lang=c++
typename enable_if
<
(is_same
<
typename _VSTD::remove_const::type,
typename _VSTD::remove_const<_SourceTp>::type
>::value
#ifndef _LIBCPP_CXX03_LANG
|| !__has_construct::value
#endif
) &&
is_trivially_move_constructible<_DestTp>::value,
void
>::type
```
But that doesn't look readable to me, so I've introduced ad-hoc ternary logic
with `__has_construct_missing`.
Comment at: libcxx/include/memory:1673-1677
+(is_same
+ <
+typename _VSTD::remove_const::type,
+typename _VSTD::remove_const<_SourceTp>::type
+ >::value
erik.pilkington wrote:
> I'm not sure if this is correct. Previously, we only selected this overload
> if the allocator didn't have a construct() member, or if it was a
> std::allocator, in which case we know construct() just called in-place new.
> With this patch, we would select this overload for a custom allocator that
> overrode construct() so long as the value_types matched. I think the right
> behaviour for the custom allocator case would be to use the construct()
> member instead of memcpying.
Thanks for pointing out the case with custom allocator. My expectation is that
it should use construct() instead of memcpy, I agree with you. Will check
further and plan to add a test.
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
erik.pilkington added a comment.
Hi Volodymyr, thanks for working on this!
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+: false_type
+{
Shouldn't this be true_type?
Comment at: libcxx/include/memory:1673-1677
+(is_same
+ <
+typename _VSTD::remove_const::type,
+typename _VSTD::remove_const<_SourceTp>::type
+ >::value
I'm not sure if this is correct. Previously, we only selected this overload if
the allocator didn't have a construct() member, or if it was a std::allocator,
in which case we know construct() just called in-place new. With this patch, we
would select this overload for a custom allocator that overrode construct() so
long as the value_types matched. I think the right behaviour for the custom
allocator case would be to use the construct() member instead of memcpying.
https://reviews.llvm.org/D48342
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added a comment. Ping. https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai added a comment. Related change is https://reviews.llvm.org/D8109 For me the performance improvement was a twice faster execution on a dirty benchmark that doesn't exclude set up and tear down. https://reviews.llvm.org/D48342 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.
vsapsai created this revision.
vsapsai added reviewers: mclow.lists, EricWF.
Herald added a subscriber: christof.
We already have a specialization that will use memcpy for construction
of trivial types from an iterator range like
std::vector(int *, int *);
But if we have const-ness mismatch like
std::vector(const int *, const int *);
we would use a slow path that copies each element individually. This change
enables the optimal specialization for const-ness mismatch. Fixes PR37574.
rdar://problem/40485845
https://reviews.llvm.org/D48342
Files:
libcxx/include/memory
libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
Index: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_different_value_type.pass.cpp
@@ -0,0 +1,29 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+//
+
+// template vector(InputIter first, InputIter last);
+
+// Initialize a vector with a different value type. Make sure initialization
+// is performed with each element value, not with a memory blob.
+
+#include
+#include
+#include
+#include
+
+int main()
+{
+int array[3] = {0, 1, 2};
+std::vector v(array, array + 3);
+assert(std::fabs(v[0] - 0.0f) < FLT_EPSILON);
+assert(std::fabs(v[1] - 1.0f) < FLT_EPSILON);
+assert(std::fabs(v[2] - 2.0f) < FLT_EPSILON);
+}
Index: libcxx/include/memory
===
--- libcxx/include/memory
+++ libcxx/include/memory
@@ -1399,6 +1399,13 @@
{
};
+template
+struct __has_construct_missing
+: integral_constant::value>
+{
+};
+
template
auto
__has_destroy_test(_Alloc&& __a, _Pointer&& __p)
@@ -1467,14 +1474,26 @@
{
};
+template
+struct __has_construct_missing
+: false_type
+{
+};
+
#else // _LIBCPP_HAS_NO_VARIADICS
template
struct __has_construct
: false_type
{
};
+template
+struct __has_construct_missing
+: false_type
+{
+};
+
#endif // _LIBCPP_HAS_NO_VARIADICS
template
@@ -1622,7 +1641,7 @@
typename enable_if
<
(is_same >::value
-|| !__has_construct::value) &&
+|| __has_construct_missing::value) &&
is_trivially_move_constructible<_Tp>::value,
void
>::type
@@ -1646,23 +1665,27 @@
construct(__a, _VSTD::__to_raw_pointer(__begin2), *__begin1);
}
-template
+template
_LIBCPP_INLINE_VISIBILITY
static
typename enable_if
<
-(is_same >::value
-|| !__has_construct::value) &&
- is_trivially_move_constructible<_Tp>::value,
+(is_same
+ <
+typename _VSTD::remove_const::type,
+typename _VSTD::remove_const<_SourceTp>::type
+ >::value
+|| __has_construct_missing::value) &&
+ is_trivially_move_constructible<_DestTp>::value,
void
>::type
-__construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2)
+__construct_range_forward(allocator_type&, _SourceTp* __begin1, _SourceTp* __end1, _DestTp*& __begin2)
{
-typedef typename remove_const<_Tp>::type _Vp;
+typedef typename remove_const<_DestTp>::type _Vp;
ptrdiff_t _Np = __end1 - __begin1;
if (_Np > 0)
{
-_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np * sizeof(_Tp));
+_VSTD::memcpy(const_cast<_Vp*>(__begin2), __begin1, _Np * sizeof(_DestTp));
__begin2 += _Np;
}
}
@@ -1686,7 +1709,7 @@
typename enable_if
<
(is_same >::value
-|| !__has_construct::value) &&
+|| __has_construct_missing::value) &&
is_trivially_move_constructible<_Tp>::value,
void
>::type
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
