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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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:
> var-const wrote:
> > ldionne wrote:
> > > 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.
> > +1, thanks a lot for the detailed analysis!
> > 
> > I have marked this patch as `[NFC]` because it simply changes the mechanism for achieving the same effect (making sure bad allocators don't cause hard errors).
> > 
> > We have to use the two-argument version because the single-argument version causes compilation errors on test code like `static_assert(std::is_default_constructible<std::vector<int, BadAlloc>>)`.
> > We have to use the two-argument version because the single-argument version causes compilation errors on test code like `static_assert(std::is_default_constructible<std::vector<int, BadAlloc>>)`.
> 
> IIUC, that's expected and a good thing. A user who instantiates `std::vector<int, BadAlloc>` is in IFNDR-land and //deserves// a hard error, just as if they instantiated `std::vector<int&, std::allocator<int&>>` or `std::function<int*>`.
> I don't think we should jump through any hoops to either permit or reject that bad code: let's just do whatever seems natural. I'm not necessarily agitating in favor of the single-argument version, just disagreeing with the conclusion that we //have to// do the two-argument version in order to accept this IFNDR code. In particular, let's not add any tests that rely on our specific behavior in this case.
I think I agree with this. I guess we should be using the one argument version in that case, to avoid promoting the use of something crazy like `std::vector<int, bad_alloc>`. It's just unfortunate that it will work with CTAD through this kind of complex chain of things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114311