[libcxx-commits] [PATCH] D112904: [libc++] P0433R2: test that deduction guides are properly SFINAEd away.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks a lot for the patch! I have some requests for changes but overall this is looking pretty good!



================
Comment at: libcxx/docs/Status/Cxx17Papers.csv:97
 "`P0430R2 <https://wg21.link/P0430R2>`__","LWG","File system library on non-POSIX-like operating systems","Kona","|Complete|","7.0"
-"`P0433R2 <https://wg21.link/P0433R2>`__","LWG","Toward a resolution of US7 and US14: Integrating template deduction for class templates into the standard library","Kona","|In Progress| [#note-P0433]_","7.0"
+"`P0433R2 <https://wg21.link/P0433R2>`__","LWG","Toward a resolution of US7 and US14: Integrating template deduction for class templates into the standard library","Kona","|Complete|","13.0"
 "`P0452R1 <https://wg21.link/P0452R1>`__","LWG","Unifying <numeric> Parallel Algorithms","Kona","",""
----------------
The next release this gets in will be 14.0, not 13.0! 13.0 is the current latest release.


================
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.
----------------
You can replace the link once you have a LWG issue number!


================
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>;
----------------
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.


================
Comment at: libcxx/include/set:477
     typedef allocator_traits<allocator_type>                  __alloc_traits;
-    typedef typename __base::__node_holder                    __node_holder;
 
----------------
I checked and this wasn't used anywhere -- that's why it's being removed right?


================
Comment at: libcxx/test/std/containers/associative/set/set.cons/deduct.pass.cpp:188-190
+    AssociativeContainerDeductionGuidesSfinaeAway<std::set, int>();
+
+    return 0;
----------------
This file is indented at 2 columns.


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


================
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>;
----------------
This is never used, and that's why the GCC build is failing. Did you forget a test case?


================
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
+
----------------
Is there a reason why this one isn't based on SFINAE checks like the container tests?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112904