[libcxx-commits] [PATCH] D112904: [libc++] P0433R2: test that deduction guides are properly SFINAEd away.
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Nov 1 16:37:03 PDT 2021
var-const added inline comments.
================
Comment at: libcxx/include/__memory/allocator_traits.h:352-354
+// A version of `allocator_traits` for internal usage that SFINAEs away if the
+// given allocator doesn't have a nested `value_type`. This helps avoid bad
+// instantiations in containers.
----------------
ldionne wrote:
> You can replace the link once you have a LWG issue number!
Done, thanks!
================
Comment at: libcxx/include/deque:1269
+ typedef typename __base::const_iterator const_iterator;
+ typedef typename __allocator_traits<allocator_type>::size_type size_type;
+ typedef typename __base::difference_type difference_type;
----------------
Quuxplusone wrote:
> I don't believe this is needed. By the time we are instantiating the members of `deque<T, allocator_type>`, we are guaranteed that `allocator_type` is an Allocator type.
Sorry, I actually had a comment about this but forgot to post it before sending the patch out for review. Unfortunately, both of these changes (not using `__base` here and using the newly-added `__allocator_traits`) are necessary -- without them, the compiler would attempt to instantiate `allocator_traits` with `allocator_type`, which would result in instantiation errors if the given `allocator_type` doesn't have `value_type`. This affects all sequential containers and leads to verbose errors when trying to deduce from a bad allocator rather than silently sfinae'ing away a bad invocation.
I can dig more into this if you feel it warrants investigation.
================
Comment at: libcxx/include/deque:1572
class _Alloc = allocator<__iter_value_type<_InputIterator>>,
+ class = enable_if_t<__is_cpp17_input_iterator<_InputIterator>::value>,
class = enable_if_t<__is_allocator<_Alloc>::value>
----------------
ldionne wrote:
> Quuxplusone wrote:
> > FWIW, `__iter_value_type<_InputIterator>` means `typename iterator_traits<_InputIterator>::value_type`, which is ill-formed (SFINAE-friendly) unless `_InputIterator` is an iterator type. However, I guess what you're doing here is protecting against the possibility that `_InputIterator` could be an //output// iterator? ...But you have added no tests for that new behavior, AFAICT.
> >
> > Please replace all your new tests with something simple that demonstrates the new behavior, e.g. https://godbolt.org/z/aKeh1fE9Y
> > ```
> > template<class... Args>
> > auto deque_has_ctad_from(int, Args&&... args) -> decltype(std::deque(std::forward<Args>(args)...), true) { return true; }
> > template<class... Args>
> > bool deque_has_ctad_from(long, Args&&...) { return false; }
> >
> > int main() {
> > std::vector<int> v;
> > auto out = std::back_inserter(v);
> > assert(!deque_has_ctad_from(42, out, out));
> > }
> > ```
> FWIW, the Standard does not request that we check for anything more than "an integral type is not an input iterator". I agree we should change the tests that currently pass a `struct BadIter { };` to make them pass a `using BadIter = some-output-iterator;`. And we need to mark those as libc++ specific, since that's technically just our QOI.
For adding this check, my main motivation was to make the requirement explicit, and also to be consistent with other similar checks. It seems to me that the fact that `__iter_value_type` also SFINAEs away "bad" input iterators is arguably an implementation detail, or at the very least, it's not as obvious without knowing the implementation as an explicit check.
I changed the tests that deal with bad iterators to use output iterators and also, where possible, to use integral types. PTAL.
================
Comment at: libcxx/include/map:229
+ class Allocator = allocator<iter_to_alloc_t<InputIterator>>>
+map(InputIterator, InputIterator, Compare = Compare(), Allocator = Allocator())
+ -> map<iter_key_t<InputIterator>, iter_val_t<InputIterator>, Compare, Allocator>;
----------------
ldionne wrote:
> Those should be marked as being new in `// C++17`. This applies elsewhere, so you might want to do a NFC patch that adjusts that in all the containers, or just fix it everywhere in this commit. I think both are reasonable, fixing everything right now is probably less churn so I'd go for that.
Fixed everything in this patch.
================
Comment at: libcxx/include/set:477
typedef allocator_traits<allocator_type> __alloc_traits;
- typedef typename __base::__node_holder __node_holder;
----------------
ldionne wrote:
> I checked and this wasn't used anywhere -- that's why it's being removed right?
Sorry, I wrote a comment about it but forgot to post it. Yes, I was investigating a compiler error stemming from the instantiation of `__base` requested here and noticed that `__node_holder` appears unused, so it seemed reasonable to remove it. I can split it into a separate small patch if you prefer.
================
Comment at: libcxx/include/set:504
typedef key_type value_type;
- typedef _Compare key_compare;
+ typedef __identity_t<_Compare> key_compare;
typedef key_compare value_compare;
----------------
Without this change (which is used in `map` and `multimap`) certain incorrect invocations try to do instantiations that depend on `_Compare`, leading to verbose errors.
================
Comment at: libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/deduct.pass.cpp:251-263
+ // (iter, iter)
+ // (iter, iter, comp)
+ // (iter, iter, comp, cont)
+ //
+ // (iter, iter, alloc)
+ //
+ // (iter, iter, comp, alloc)
----------------
ldionne wrote:
> I don't feel like this list adds very much since you commented the individual test cases below (thanks a bunch for doing that BTW). Feel free to keep them if you want, that's just my .02.
Yeah, I was on the fence about leaving these in -- removed now.
================
Comment at: libcxx/test/std/containers/container.adaptors/priority.queue/priqueue.cons/deduct.pass.cpp:273
+ using AllocAsComp = Alloc;
+ using AllocAsCont = Alloc;
+ using DiffAlloc = test_allocator<int>;
----------------
ldionne wrote:
> This is never used, and that's why the GCC build is failing. Did you forget a test case?
Thanks for spotting! Yes, looks like I missed 3 test cases.
================
Comment at: libcxx/test/std/containers/container.adaptors/stack/stack.cons/deduct.pass.cpp:140
+ // Deduction guides should be SFINAE'd away when given:
+ // - a "bad" allocator (that is, a type not qualifying as an allocator);
----------------
`stack` and `queue` have the same tests, but it didn't seem worthwhile to refactor them out in a separate function.
================
Comment at: libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.ctor/deduct.fail.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14
+
----------------
ldionne wrote:
> Is there a reason why this one isn't based on SFINAE checks like the container tests?
For containers, the requirement is for deduction guides to `not participate in overload resolution`, but for unique pointers, the requirement is that `If class template argument deduction would select the function template corresponding to this constructor, then the program is ill-formed`. I interpreted `ill-formed` as "resulting in a hard error" -- please let me know if this is incorrect.
================
Comment at: libcxx/test/support/deduction_guides_sfinae_checks.h:22
+template<template<typename ...> class Container>
+constexpr void SequenceContainerDeductionGuidesSfinaeAway() {
+ using Alloc = std::allocator<int>;
----------------
Please let me know if you'd prefer shorter names for these functions.
================
Comment at: libcxx/test/support/sfinaes_away.h:15-28
+// `SFINAEs_away` template variable checks whether the template arguments for
+// a given template class `Instantiated` can be deduced from the given
+// constructor parameter types `CtrArgs` using CTAD.
+
+template<template<typename ...> class Instantiated, class ...CtrArgs,
+ class = decltype(Instantiated(std::declval<CtrArgs>()...))>
+std::false_type SFINAEs_away_impl(int);
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > IMO this utility is a bit too narrow in usefulness to make it its own header, since it only works for checking that a class template's CTAD SFINAEs away. If this were for checking that a general expression SFINAEs away, then I'd be in favour of keeping it standalone.
> > >
> > > I would suggest either moving it to `deduction_guides_sfinae_checks.h` or generalizing it.
> > +1 and more so: I don't think this PR should add //any// new headers. Each "deduct.pass.cpp" should just introduce a four-line `bool foo_has_ctad_from(42, args...)` (for the appropriate container-name `foo`).
> I disagree with that. I looked at the PR in a previous stage where the checks were duplicated, and there was *a lot* of code duplication. Since most of these requirements are specified in http://eel.is/c++draft/container.requirements.general, I think it makes sense to colocate the tests the way they are right now.
Moved `SFINAEs_away` to `deduction_guides_sfinae_checks.h`.
I'd prefer to avoid duplication if possible. I had each test file have its own tests and definitions of everything in earlier states of this patch. It was a pain to modify, and I made some inevitable bugs forgetting to apply a change to all the relevant files.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112904/new/
https://reviews.llvm.org/D112904
More information about the libcxx-commits
mailing list