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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/functional:219
 
+struct identity;
+
----------------
Quuxplusone wrote:
> Please move this up to directly after `bit_not` (and before `unary_negate`).
> The synopses of the bitwise operators were all cut-and-paste-typoed; I just pushed a fix (changing comments only) so now there //is// a `bit_not` to put this after. :)
Please add comment that this has been added in C++20.


================
Comment at: libcxx/include/functional:3240
+    template<class _Tp>
+    constexpr _Tp&& operator()(_Tp&& __t) const noexcept
+    {
----------------
Can you add a test that this function is `noexcept`?


================
Comment at: libcxx/test/std/utilities/function.objects/func.identity/identity.pass.cpp:43
+  return result;
+}
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98151