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

Tue Nov 30 06:29:13 PST 2021

Quuxplusone added a comment.

No objection from me, modulo that the C++03 `forward_list` test is still failing for some reason.

================
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)
+    {
----------------
ldionne wrote:
> 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.
Ah, I'd missed that line. Okay, there's less of a maze than I thought. :)
I still think it's confusingly subtle that the `_Allocator` on line 1293 is understood by the compiler as the same type as the `allocator_type` on line 1294, but I have no concrete suggestion to make it better. (We could use `allocator_type` on line 1293: `_DependentAlloc = allocator_type`: that's still kinda weird. Or we could use `_Allocator` on line 1294: `size_type __n, const value_type& __v, const _Allocator& __a`: that's also weird.) No action required AFAIC.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114311