[libcxx-commits] [PATCH] D114311: [libcxx] Make sequential containers slightly more SFINAE-friendly during CTAD.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

For testing, I think you've got the right idea: the only way to test this (besides ugly/fragile .verify.cpp tests) will be SFINAE. You just implemented `SequenceContainerDeductionGuidesSfinaeAway` in a previous patch, right? So shouldn't this just be a one- or two-line addition to that function?

  // Cannot deduce from (size_type, value_type, BAD_alloc)
  static_assert(SFINAEs_away<Container, size_t, void*, BadAlloc>);

If that doesn't work as a regression-test, then I'm confused about the effect of this patch.



================
Comment at: libcxx/include/deque:1275
+    typedef _VSTD::reverse_iterator<iterator>              reverse_iterator;
+    typedef _VSTD::reverse_iterator<const_iterator>        const_reverse_iterator;
 
----------------
Why the extra space on all these lines? Just to highlight this section in Phab? (I would guess "removing hard tabs," but we reject hard tabs at buildkite time.)


================
Comment at: libcxx/include/deque:1293-1294
+
+    template <class _MaybeAllocator = _Allocator, class = __enable_if_t<__is_allocator<_MaybeAllocator>::value> >
+    deque(size_type __n, const value_type& __v, const allocator_type& __a) : __base(__a)
+    {
----------------
This change seemed OK until I realized that I didn't understand the maze that gets us from `_Allocator` (on line 1293) to `allocator_type` (on line 1294). From the context, I assume that `allocator_type` here is understood by the compiler as an alias for `__base::allocator_type` i.e. `__deque_base<_Tp, _Allocator>::allocator_type` i.e. `_Allocator` i.e. //deque's own// `_Allocator` parameter, and then CTAD applies. I just don't fully understand how the CTAD machinery is working — it has to instantiate `__deque_base<SomeType, BadAllocator>` in order to find out that `__deque_base<_Tp, _Allocator>::allocator_type` is `_Allocator`?
But when I try that myself on Godbolt, the compiler //can't// thread that maze.
https://godbolt.org/z/oMTqqvhhW
I must be doing something wrong in that Godbolt, but what?


================
Comment at: libcxx/include/string:855
 
-    template <class = __enable_if_t<__is_allocator<_Allocator>::value, nullptr_t> >
+    template <class _MaybeAllocator = _Allocator, class = __enable_if_t<__is_allocator<_MaybeAllocator>::value, nullptr_t> >
         _LIBCPP_INLINE_VISIBILITY
----------------
While you're here, please remove `, nullptr_t` from the existing default argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114311