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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
