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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/forward_list:670
 #if _LIBCPP_STD_VER > 11
     explicit forward_list(size_type __n, const allocator_type& __a);
 #endif
----------------
(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).


================
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);
+    }
----------------
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114311