Support std::identity

marked the checklist item Pipeline passing as completed

mentioned in merge request pdelab/dune-pdelab!427 (closed)

Can you add a Changelog entry?

In my opinion this code is really dangerous. As far as I understand, this will bind a reference to an outlived temporary if you do:

struct Foo {...};
auto&& foo = std::identity(Foo());

In this situation T is deduced to be plain Foo and the created temporary is bound to the argument of operator() which is an r-value reference of type T&& aka Foo&&. This is OK and prolongs lifetime of the temporary as long as this reference lives. Now std::forward casts this to an r-value reference T&& which is returned. The problem is that the lifetime of the object ends with the first bound-to reference which is the operator() argument. Hence, the foo variable outside will point to a non-existing object.

I looked this up and also found the proposed implementation in the ranges TS. If this is really the desired behaviour it should be carefully documented that you're (surprisingly) not allowed to use r-values as arguments. In this case I explicitly ask you to add the corresponding documentation.

All of this holds true unless there was a language change in C++20 making this valid. In this case the return type should better be T here, which would avoid the outlined problem in pre C++20.

The current (final before ISO member feedback) C++20 draft states in 20.14.11:

Effects: Equivalent to: return std::forward(t);

So this seems to be intended behavior.

It definitely is dangerous, it seems to be built mostly for direct consumption in an outer function call, where it works correctly, e.g.:

#include <iostream>

struct Foo {
  Foo() { std::cout << "construct" << std::endl; }
  Foo(const Foo&) { std::cout << "copy construct" << std::endl; }
  Foo(Foo&&) { std::cout << "move construct" << std::endl; }
  ~Foo() { std::cout << "deconstruct" << std::endl; }
};


struct identity {
  template<class T>
  constexpr T&& operator()(T&& t ) const noexcept {return std::forward<T>(t);}
};

template<typename T>
T&& print(T&& arg)
{
  std::cout << "print" << std::endl;
  return std::forward<T>(arg);
}


int main()
{
  auto id = identity();
  print(id(Foo()));
  std::cout << "next" << std::endl;
}

This prints: construct print deconstruct next, so there is no problem with the lifetime in this context.

I just checked this against g++-8.3 -std=c++17. Using

struct Foo {
  Foo() { std::cout << "construct" << std::endl; }
  Foo(const Foo&) { std::cout << "copy construct" << std::endl; }
  Foo(Foo&&) { std::cout << "move construct" << std::endl; }
  ~Foo() { std::cout << "deconstruct" << std::endl; }
};

...

auto id = identity();
const auto& foo = id(Foo());
std::cout << "next" << std::endl;

will print 'construct', 'destruct', 'next' as expected.

Thanks for the check, do you think I should put this into a test?

The use case proposed by @smuething is OK and seems to be what it was invented for. In general passing r-value references should not be any problem. If the object lives on the stack, it will always outlive any reference obtained by operator() unless you explicitly shot yourself in the foot.

As mentioned above, the problem is passing r-values and not r-value references. Coming with the ranges TS, the main purpose seems to be to act as a predicate in range algorithms. Unfortunately this may also lead to delicate problems with generated on the fly ranges. While this one is OK

auto id = identity;
for(auto&& value : range)
  std::cout << value << std::endl;

the more verbose one is not

auto id = identity;
for(auto it = range.begin(); it != range.end(); ++it) {
  auto&& value = id(*it);
  std::cout << value << std::endl;
}

Although its generic name may indicate something else, this really is an expert utility and we shoud in detail document, what's allowed and what's not to avoid people running into hard to detect errors.

Maybe it's really a good idea to document the issue with a test:

struct Foo {
  static int count;
  Foo() { ++count; }
  Foo(const Foo&) { ++count; }
  Foo(Foo&&) { ++count }
  ~Foo() { --count; }
};
int Foo::count = 0;
...

auto id = identity();
const auto& foo = id(Foo());
if (Foo::count > 0) std::cerr << "Some meaningful message." << std::endl;

@smuething Sorry I misread your example. You are passing an r-value, but don't store the return value of operator(). That's still OK, because the object will live during the evaluation of the current expression.

added 3 commits

8dd6444a - Add documentation to identity
9e72640c - Include test for identity
ebbb1857 - Add CHANGELOG for identity

Compare with previous version

@carsten.graeser Alright, how about now?

marked the checklist item Added entry to CHANGELOG.md as completed

marked the checklist item Added test as completed

added 1 commit

a2044278 - Remove assert from test

Compare with previous version

added Ready feature labels

@smuething @carsten.graeser Is this ready to merge? Or Post 2.7?

LGTM

added 129 commits

a2044278...7f719ae9 - 124 commits from branch master
7534698c - Include Std::identity
7affda68 - Add documentation to identity
15ae0f30 - Include test for identity
7e7eee1e - Add CHANGELOG for identity
75c81fe9 - Remove assert from test

Compare with previous version

merged

mentioned in commit 9bb1b60a

Support std::identity

What does this MR do?

Can this MR be accepted?

Related issues

Activity

Support std::identity

What does this MR do?

Can this MR be accepted?

Related issues

Merge request reports

Activity