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

Konstantin Varlamov via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Nov 30 00:18:04 PST 2021


var-const added inline comments.


================
Comment at: libcxx/include/deque:1275
+    typedef _VSTD::reverse_iterator<iterator>              reverse_iterator;
+    typedef _VSTD::reverse_iterator<const_iterator>        const_reverse_iterator;
 
----------------
Quuxplusone wrote:
> Why the extra space on all these lines? Just to highlight this section in Phab? (I would guess "removing hard tabs," but we reject hard tabs at buildkite time.)
Removed (I think).


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


================
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)
+    {
----------------
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;
```


================
Comment at: libcxx/include/forward_list:670
 #if _LIBCPP_STD_VER > 11
     explicit forward_list(size_type __n, const allocator_type& __a);
 #endif
----------------
ldionne wrote:
> 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.
(1) I'm not sure. My primary concern was with the overloads that overlap with explicit deduction guides, and I haven't seen a case where this two-argument overload would cause issues. I'd rather keep the scope limited because expanding it later if necessary is straightforward whereas removing the additional constraints, should the need arise, would likely break users.



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


================
Comment at: libcxx/include/string:855
 
-    template <class = __enable_if_t<__is_allocator<_Allocator>::value, nullptr_t> >
+    template <class _MaybeAllocator = _Allocator, class = __enable_if_t<__is_allocator<_MaybeAllocator>::value, nullptr_t> >
         _LIBCPP_INLINE_VISIBILITY
----------------
Quuxplusone wrote:
> While you're here, please remove `, nullptr_t` from the existing default argument.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114311



More information about the libcxx-commits mailing list