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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/vector:492
+    typedef typename __base::const_reference                         const_reference;
+    typedef typename __allocator_traits<allocator_type>::size_type   size_type;
+    typedef typename __base::difference_type                         difference_type;
----------------
Quuxplusone wrote:
> I still don't get this diff; but is it possible that it'll be a moot point after D112976?
Without this change, the compiler errors out trying to instantiate `allocator_traits` with an incorrect allocator instead of silently SFINAE'ing away the deduction guide. However, like you said, it's a moot point after the other patch lands, so I don't think we need to spend time investigating this issue.


================
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
+
----------------
Quuxplusone wrote:
> var-const wrote:
> > ldionne wrote:
> > > var-const wrote:
> > > > 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.
> > > Oh, then this is correct. It's a really unusual specification for this kind of failure though.
> > I looked into this more closely, and there's actually been a change on this between C++17 and C++20.
> > 
> > In C++17, the requirement is:
> > ```
> > If class template argument deduction would select the function template corresponding to this constructor, then the program is ill-formed.
> > ```
> > However, [P1460](wg21.link/p1460) changed this in C++20 to:
> > ```
> > Mandates: This constructor is not selected by class template argument deduction.
> > ```
> > Now the words `ill-formed` are gone, and with the new wording, it seems like these constructors should just SFINAE away without causing a hard error. Moreover, this seems like a slight change in behavior. I don't see any discussion of this case in the paper, and I presume the new behavior is slightly more user-friendly.
> > 
> > It seems reasonable to me to only implement and test the new behavior, including in C++17 mode as well (FWIW, it was already implemented in a SFINAE-friendly way). Because of that, I replaced the previous `deduct.fail.cpp` file with a new `deduct.pass.cpp` test that uses `SFINAEs_away`. Let me know what you think!
> "Mandates" always means "static_assert, not SFINAE." When the standard means SFINAE, it uses "Constraints" instead. However, I think this is an editorial defect in p1460's changes: it replaced a soft "Remark" with a hard "Mandates." It should have said:
> 
> `Remarks: This constructor is never selected by class template argument deduction.`
> 
> or simply
> 
> `unique_ptr(type_identity_t<pointer> p)`
> 
> libstdc++ and MSVC both interpret the intent as "this constructor shouldn't contribute to CTAD," which matches what you settled on here, IIUC.
> I'll email LWG suggesting a new issue to clarify this wording.
> I'll email LWG suggesting a new issue to clarify this wording.

Thanks!




================
Comment at: libcxx/test/support/deduction_guides_sfinae_checks.h:23-32
+template<template<typename ...> class Instantiated, class ...CtrArgs,
+    class = decltype(Instantiated(std::declval<CtrArgs>()...))>
+std::false_type SFINAEs_away_impl(int);
+
+template<template<typename ...> class Instantiated, class ...CtrArgs>
+std::true_type SFINAEs_away_impl(...);
+
----------------
Quuxplusone wrote:
> Even after seeing @ldionne's comment, personally, I'd still write these tests without any additional headers. For a sequence container, we'd have this:
> ```
> template<class... Args>
> auto deque_has_no_ctad_from() -> decltype(std::deque(std::declval<Args>()...));
> template<class... Args>
> constexpr bool deque_has_no_ctad_from() { return true; }
> 
> int main(int, char**)
> {
>     // all the deduct.pass.cpp tests, and then these lines:
> 
>     using OutIt = std::insert_iterator<std::vector<int>>;
>     struct NotAnAllocator {};
> 
>     LIBCPP_STATIC_ASSERT(deque_has_no_ctad_from<OutIt, OutIt>());
>     LIBCPP_STATIC_ASSERT(deque_has_no_ctad_from<OutIt, OutIt, std::allocator<int>>());
>     static_assert(deque_has_no_ctad_from<int*, int*, NotAnAllocator>());
> }
> ```
> And for an associative container like `map` we'd have this:
> ```
> template<class... Args>
> auto map_has_no_ctad_from() -> decltype(std::map(std::declval<Args>()...));
> template<class... Args>
> constexpr bool map_has_no_ctad_from() { return true; }
> 
> int main(int, char**)
> {
>     // all the deduct.pass.cpp tests, and then these lines:
> 
>     using VT = std::pair<const int, int>;
>     using OutIt = std::insert_iterator<std::vector<int>>;
>     struct NotAnAllocator {};
> 
>     static_assert(map_has_no_ctad_from<int, int>());
>     static_assert(map_has_no_ctad_from<int, int, std::less<int>>());
>     static_assert(map_has_no_ctad_from<int, int, std::allocator<VT>>());
>     static_assert(map_has_no_ctad_from<int, int, std::less<int>, std::allocator<VT>>());
>     LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt>());
>     LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::less<int>>());
>     LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::allocator<VT>>());
>     LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::less<int>, std::allocator<VT>>());
>     static_assert(map_has_no_ctad_from<VT*, VT*, std::allocator<VT>, std::allocator<VT>>());
>     static_assert(map_has_no_ctad_from<VT*, VT*, std::less<int>, NotAnAllocator>());
>     LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<std::initializer_list<VT>, std::allocator<VT>, std::allocator<VT>>);
>     LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<std::initializer_list<VT>, std::less<int>, NotAnAllocator>);
> }
> ```
> I just don't see the need for 300 lines of code off in test/support/ (plus another 100 lines in the priority_queue test) when we could have ~20 lines per container, right there in each container's test directory.
A better example would be the unordered containers which have 36 test cases per container. While we could reduce the line count by omitting comments, I find this state less readable and much harder to modify or to verify that every case is covered. While I was working on this patch, I had to change the tests multiple times, and I found comments to be very helpful when making changes. Moreover, duplicating each change at least 4 times is annoying and error-prone. Finally, if the tests were duplicated, the reader might have to manually compare different files to make sure they are exactly the same -- I have found several inconsistencies between similar duplicated tests while working on this patch. For these reasons, I'd prefer to keep the current state, even if it is more verbose.

I don't see the fact that the tests covering the same aspect of functionality are all in one place as a downside, necessarily. After all, most of these requirements are placed by the Standard on different classes of containers, not on individual containers.

What is the downside of adding a new header? I'm not sure why significant code duplication would be preferable, and my experience working on this patch suggests otherwise.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112904