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

Mon Nov 22 10:03:32 PST 2021

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

My understanding is that this is effectively a NFC, since you were previously using `__allocator_traits` to achieve the exact same effect. This is only making all the containers consistent. That's why there are no test changes, right?

================
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)
----------------
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.

================
Comment at: libcxx/include/forward_list:670
 #if _LIBCPP_STD_VER > 11
     explicit forward_list(size_type __n, const allocator_type& __a);
 #endif
----------------
Quuxplusone wrote:
> (1) Don't you need to guard this one on `__is_allocator<allocator_type>`, too?  I think we're missing this case in `forwardlist.cons/deduct.fail.cpp`.
> (2) Personally I'd be interested in just disabling the implicit guides for //all// these ctors (via `__identity_t`), and using explicit deduction guides for everything instead. Seems like explicit deduction guides would be a lot less error-prone and a lot more self-documenting, and I believe we'd be allowed to do it under the As-If Rule (I don't think a user could detect our trick).
On (2), my vote would be towards keeping our implementation as close to the Standard as possible, which means keeping a mix of implicit and explicit deduction guides like we do right now. I don't like how the Standard is specified in that regard (I'd rather have explicit deduction guides only*), however at least we can more easily report issues against the Standard if we don't stray away from it, and it's easier to validate whether we are conforming.

(*) I'd be even happier if implicit deduction guides didn't even exist, but that's a different story.

================
Comment at: libcxx/include/forward_list:673-677
+    template <class _MaybeAllocator = _Alloc, class = __enable_if_t<__is_allocator<_MaybeAllocator>::value>>
+    forward_list(size_type __n, const value_type& __v, const allocator_type& __a) : base(__a)
+    {
+        insert_after(cbefore_begin(), __n, __v);
+    }
----------------
Quuxplusone wrote:
> > Yes, this works (the existing `SFINAEs_away<Container, Iter, Iter, BadAlloc>` works as well -- it would give a hard error without either this patch or the previous approach that SFINAEd away allocator_traits)
> 
> Well, an //existing// test can't be a regression test, by definition. It already passed without this patch. :) Unless you're calling this PR "NFC [no functional change [intended]]", then it ought to have some functional effect and that means we ought to be able to write a regression test for that specific effect.
> 
> > What I meant to say is that it's hard to test for the difference between having one or two arguments in the template function:
> > ``` 
> > // This is, IIUC, the correct form because it makes sure that `__is_allocator` is called on a deduced type.
> > template <class _MaybeAllocator = _Allocator, class = __enable_if_t<__is_allocator<_MaybeAllocator>::value> >
> > // However, this works as well, even though `_Allocator` is not, AFAICT, a deduced type. It seems to me that
> > // it shouldn't work and perhaps Clang is being permissive here -- otherwise I must be missing something.
> > template <class = __enable_if_t<__is_allocator<_Allocator>::value> >
> > ```
> > I couldn't think of a great way to test this difference. 
> 
> I think it's going to be difficult/impossible in this case, yeah. Here's a reduced test case:
> https://godbolt.org/z/cbaM9hsxj
> Normally the one-template-argument version is //wrong// because the `__enable_if_t` it will be instantiated relatively eagerly. (As soon as the class definition is instantiated, it'll instantiate all of the member functions' //declarations//, which will include the one that depends on a non-existent `enable_if_t<false>` type.)
> However, in this case, CTAD happens "more eagerly than that." //First// we transform the constructor template declaration into CTAD's fictional function template declaration, discover that it'd be ill-formed, and CTAD fails; so then we decide not to instantiate the class definition at all.
> However, if someone later attempts to instantiate `vector<int, NotAnAllocator>` by name, the single-template-argument ctor will cause failure, where the two-template-argument ctor would not cause failure.
> //However//, in this case, we don't care because instantiating `vector<int, NotAnAllocator>` is not allowed.
> Basically, the two-template-argument version `template<class Y=X, class=enable_if_t<f(Y)>>` is absolutely needed when `!f(X)` is legal but you're just trying to SFINAE away part of the class's API in that case. When `!f(X)` is flat-out illegal and you're just trying to teach that fact to CTAD, you can get away with the one-template-argument version.
Interesting, thanks for your analysis. I still think we should go with the two argument case for simple consistency and to avoid confusion. The fact that CTAD deduction guide generation will not cause the class to be instantiated is IMO an interesting detail, but one what folks shouldn't have to think about when reading this code.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114311