[libcxx-commits] [PATCH] D98151: [libcxx] adds std::identity to <functional>

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/utilities/function.objects/func.identity/identity.pass.cpp:43
+  return result;
+}
+
----------------
Mordante wrote:
> Quuxplusone wrote:
> > Quuxplusone wrote:
> > > Please, as @ldionne would say, "add some simple tests." :)
> > > I think given the simplicity of `std::identity`, the entire test suite for it should look like this:
> > > ```
> > > #include "support/moveonly.h"
> > > 
> > > constexpr bool test() {
> > >     std::identity id;
> > >     int i = 42;
> > >     assert(id(i) == 42);
> > >     assert(id(std::move(i)) == 42);
> > > 
> > >     MoveOnly m1 = 2;
> > >     MoveOnly m2 = id(std::move(m1));
> > >     assert(m2.value == 2);
> > > 
> > >     assert(&id(i) == &i);
> > >     assert(&id(std::move(i)) == &i);
> > >     assert(&id(std::move(m1) == &m1);
> > >     static_assert(&id(id) == &id);
> > > 
> > >     const std::identity idc;
> > >     assert(idc(1) == 1);
> > >     assert(std::move(id)(1) == 1);
> > >     assert(std::move(idc)(1) == 1);
> > > 
> > >     id = idc;  // test assignability
> > > 
> > >     static_assert(std::is_same_v<decltype(id(i)), int&>);
> > >     static_assert(std::is_same_v<decltype(id(std::declval<int&&>())), int&&>);
> > >     static_assert(std::is_same_v<decltype(id(std::declval<const int&>)), const int&>);
> > >     static_assert(std::is_same_v<decltype(id(std::declval<const int&&>)), const int&&>);
> > >     static_assert(std::is_same_v<decltype(id(std::declval<volatile int&>)), volatile int&>);
> > >     static_assert(std::is_same_v<decltype(id(std::declval<volatile int&&>)), volatile int&&>);
> > > 
> > >     return true;
> > > }
> > > 
> > > int main(int, char**) {
> > >     test();
> > >     static_assert(test());
> > > 
> > >     return 0;
> > > }
> > > ```
> > > 
> > > I don't think the test suite should depend on C++20 nor concepts. I don't think we need any more lines of test code than what I've written here.
> > ...well, and also a test that `std::identity::is_transparent` is a typename.
> > 
> >     static_assert(std::is_same_v<std::void_t<std::identity::is_transparent>, void>);
> > 
> > should do the trick.
> @Quuxplusone The feature is C++20 so we can't drop that, but I agree with the concepts.
> Since it's unspecified what `is_transparent` is I think it would be better not to test it's value. Doesn't the test at line 23 already test what you want?
> 
> @cjdb I'm also in favour of a simpler test for this feature.
> Since it's unspecified what `is_transparent` is I think it would be better not to test its value.

I agree; note the `void_t` in my suggestion. I believe this accomplishes the "it must be a type but I don't care what type it is" goal.

And this plays into my "not depend on C++20" request. The test on line 23 //does// test what I want, but it uses C++20 core-language Concepts features to do it. I prefer my suggestion because it doesn't depend on the compiler to implement C++20 Concepts; in that sense I'm requesting a "C++17 test for a C++20 library feature." I know we can't remove `UNSUPPORTED: c++17`; I just think it's cleaner to use the more "basic" language features anywhere we can.  For the same reason, my proposed test cases use e.g. `std::is_same_v<decltype(id(i)), int&>` and not `std::same_as<decltype(id(i)), int&>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98151