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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM but the C++03 CI will have to pass. I haven't looked too closely but I don't think it's a major issue.



================
Comment at: libcxx/include/deque:1293
+
+    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)
----------------
var-const wrote:
> var-const wrote:
> > ldionne wrote:
> > > Quuxplusone wrote:
> > > > 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?
> > > It's not really a "maybe" allocator. It's definitely `_Allocator`. I've also used `_DependentAlloc` in the past (e.g. in `<tuple>`) for the same purpose, in case you like that better.
> > What I was trying to convey is that the type is being tested for whether it is an allocator. Changed to `_DependentAlloc`.
> I think that, unlike the other type aliases, `allocator_type` doesn't depend on the base class. See line 1257:
> ```
> typedef _Allocator allocator_type;
> ```
Oh, I see what you were doing now. Hmm I guess I'm neutral on the rename then. I guess I'm still slightly happier with `_DependentAlloc` since it's a tiny bit more consistent with other places in the library.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114311