[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/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:
> var-const wrote:
> > 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.
> > 
> > What is the downside of adding a new header?
> Just that it's always easier to understand code that's in one place, as opposed to having to skip back and forth between different files to get the full picture. Also for "attribution": when a test fails, it's nice to be able to point to //the// line that failed, which should be in //the// test that failed; having asserts in .h files included from multiple tests makes that just a bit harder. Also in case of later refactoring: suppose `set` and `map` have the same CTAD guides today, but then `map` gains one new guide tomorrow; with the "one line of code per test case" version, that's a one-line diff to add the new test case, but with the "everything calls into the same .h file" version, it might require arbitrarily invasive refactoring of the shared .h file. ("Copy-on-write," in other words.)
> Basically, this is the software-engineering version of the "aliasing plus mutation" problem. Aliasing //alone// isn't a problem if everything is const; mutation //alone// isn't a problem if nothing is aliased; but combine aliasing with mutation and suddenly you have the potential for bugs and race conditions.
> Which is not to say that plenty of programmers can't deal just fine with aliasing plus mutation. I mean, C++ does it. ;) So, having stated my view, I'll now let this thread be.
Thank you for the writeup! I still think that, given the amount of duplication, having common functions is the lesser evil in this case, but I understand your concerns much better now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112904