[PATCH] D48753: [libcxx] Use custom allocator's `construct` in C++03 when available.

[Louis Dionne via Phabricator via cfe-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: jkorous.

1. Just to make sure I understand; this patch has nothing to do with https://reviews.llvm.org/D48342, they are entirely orthogonal. Is this correct?
2. Also, before this patch, the allocator's `construct` and `destroy` were NEVER called in C++03, but were called when available in C++11. After this patch, they are called when available in C++03 and in C++11. Is this correct?

If (2) is true, then I believe this patch is a worthwhile improvement in terms of quality-of-implementation, even though it's not mandated by the spec. People do have custom allocators, and this behavior change between C++03 and C++11 is quite subtle and surprising.



================
Comment at: libcxx/include/memory:1466
+template <class _Alloc, class _Pointer, class _Tp>
+struct __has_construct<_Alloc, _Pointer, _Tp, typename enable_if<
+    is_same
----------------
You can just say

```
template <class _Alloc, class _Pointer, class _Tp>
struct __has_construct<_Alloc, _Pointer, _Tp, typename __void_t<
    decltype(_VSTD::declval<_Alloc>().construct(_VSTD::declval<_Pointer>(), _VSTD::declval<_Tp>()))
>::type> : std::true_type { };
```


================
Comment at: libcxx/include/memory:1482
+    <
+        decltype((_VSTD::declval<_Alloc>().destroy(_VSTD::declval<_Pointer>()), void())),
+        void
----------------
Here,

```
template <class _Alloc, class _Pointer>
struct __has_destroy<_Alloc, _Pointer, typename __void_t<
    decltype(_VSTD::declval<_Alloc>().destroy(_VSTD::declval<_Pointer>()))
>::type> : std::true_type { };
```


================
Comment at: libcxx/include/memory:1726
+#else  // _LIBCPP_HAS_NO_VARIADICS
+    template <class _Tp, class _A0>
+        _LIBCPP_INLINE_VISIBILITY
----------------
Here,

```
template <class _Tp, class _A0, class = typename enable_if<__has_construct<allocator_type, _Tp*, _A0 const&>::value>::type>
static void __construct(...);
```

and then below

```
template <class _Tp, class _A0, class = typename enable_if<!__has_construct<allocator_type, _Tp*, _A0 const&>::value>::type>
static void __construct(...);
```

This way you don't have to call `__has_construct` at the point of call and `__construct` is slightly more reusable.



================
Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter.pass.cpp:122
+  {
+    typedef std::vector<int, cpp03_allocator<int> > C;
+    typedef C::allocator_type Alloc;
----------------
These tests are specific to libc++ because C++03 does not mandate this behavior. Please either move them into `test/libcxx` or guard them with `_LIBCPP_VERSION`. I think the former is what we normally do in these cases and should be preferred.


================
Comment at: libcxx/test/std/containers/sequences/vector/vector.cons/construct_iter_iter_alloc.pass.cpp:135
+  {
+    typedef std::vector<int, cpp03_allocator<int> > C;
+    typedef C::allocator_type Alloc;
----------------
Same as above, this is libc++ specific.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D48753/new/

https://reviews.llvm.org/D48753