Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for reference_wrappers in make_xxx #179

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Sep 13, 2015

This pull request attempts to gradually add support for reference_wrapper in the make_xxx family of functions. This is in order to fix #176.

As documented in #176, I decided to go with the "forward declare in the std:: namespace" approach, because I think this will be far better for users (let's not introduce yet another reference_wrapper class). The following containers should support reference_wrapper:

  • tuple
  • set
  • map
  • pair
  • optional
  • lazy

We also need to:

  • update the tutorial so it explains which containers can hold references, and how reference_wrapper can be used with make_xxx to create a container of references.

@ldionne ldionne force-pushed the feature/reference_wrapper branch 2 times, most recently from faceac0 to 01f0afc Compare September 13, 2015 17:09
@ldionne
Copy link
Member Author

ldionne commented Sep 13, 2015

Actually, map and set should probably not support reference_wrappers.

For map, this is because make_map takes pairs anyway, and those can hold references if needed. Also, the first element of each pair in a map is the key, which must be compile-time Comparable, which in turn means that it will be something like a hana::type, IntegralConstant or another stateless object 95% of the time.

For set, this is because the elements of the set must be compile-time Comparable, so the same reasoning applies to them as for the map's elements.

@ldionne
Copy link
Member Author

ldionne commented Sep 13, 2015

This PR is essentially feature complete, but I'm not sure about it. Actually, I'm not sure at all about the whole "storing references in containers" thing. Indeed, consider the following:

#include <boost/hana/tuple.hpp>
#include <boost/hana/tail.hpp>
namespace hana = boost::hana;

int main() {
    int i = 0, j = 1, k = 2;
    hana::tuple<int&, int&, int&> xs{i, j, k};
    hana::tuple<int, int> ys = hana::tail(xs);
    //          ^^^  ^^^ notice the decay
}

The problem is that tail is essentially implemented as make_tuple(at_c<1>(xs), at_c<2>(xs), ...). But since the elements of xs are references (not reference_wrappers), they are decayed and we end up creating a tuple of values. Of course, we could transport the fact that xs is holding references, but that would require specifying the return type of tail as something like hana::tuple<tuple_element_t<0, Xs>, ...>. I see two problems:

  1. Doing this generically is going to add complexity to the Sequence concept at least.
  2. We'll completely blow away our compile-time performance because tuple_element_t can't be implemented efficiently AFAIK.

Also note that this problem extends to basically any algorithm that returns a new sequence created with make_xxx(at_c<0>(xs), at_c<1>(xs), ...), which is a lot of algorithms.

Hence, documenting that containers can hold references and making this easier by supporting the reference_wrapper magic seems like asking for a lot of trouble.

@ldionne ldionne force-pushed the feature/reference_wrapper branch 2 times, most recently from 9d4e74d to 9c6b42c Compare October 13, 2015 05:13
@ldionne ldionne force-pushed the feature/reference_wrapper branch 2 times, most recently from 81fe957 to a7c50c5 Compare October 26, 2015 19:28
@ldionne ldionne force-pushed the feature/reference_wrapper branch 2 times, most recently from 99ff528 to cf327f5 Compare November 15, 2015 08:07
@badair
Copy link
Contributor

badair commented Nov 24, 2015

What if hana::type contained an additional type alias for the exact original template argument, without reference decay? Would this remove the need for reference wrapping? Would it be difficult to preserve this across algorithms?

@ldionne
Copy link
Member Author

ldionne commented Nov 24, 2015

What if hana::type contained an additional type alias for the exact original template argument, without reference decay?

hana::type does not decay references. More specifically, std::is_same<hana::type<T>::type, T>::value is always true.

@badair
Copy link
Contributor

badair commented Nov 24, 2015

Ok, I just realized the "rationale for stripping references" section of the documentation is about decltype_ specifically. My mistake.

@ldionne
Copy link
Member Author

ldionne commented Nov 24, 2015

Yes, it's about decltype_ only. No worries.

@ldionne ldionne force-pushed the feature/reference_wrapper branch 3 times, most recently from da2a3df to 37885cf Compare December 29, 2015 06:40
@ldionne
Copy link
Member Author

ldionne commented Nov 8, 2016

Giving an explicit example of a use case where this issue comes up, for the purpose of a discussion currently going on at the Issaquah meeting. Consider the following:

#include <tuple>

template <typename Tuple, typename U, std::size_t ...i>
auto push_back_impl(Tuple&& tuple, U&& u, std::index_sequence<i...>) {
  // if we use deduction guides with unwrapping of reference_wrappers, this
  // may end up unwrapping reference wrappers under our feet, in this totally
  // generic context
  return std::tuple{std::get<i>(std::forward<Tuple>(tuple))..., std::forward<U>(u)};
}

template <typename ...T, typename U>
auto push_back(std::tuple<T...>&& tuple, U&& u) {
  return push_back_impl(std::move(tuple), std::forward<U>(u),
                        std::make_index_sequence<sizeof...(T)>{});
}

template <typename ...T, typename U>
auto push_back(std::tuple<T...> const& tuple, U&& u) {
  return push_back_impl(tuple, std::forward<U>(u),
                        std::make_index_sequence<sizeof...(T)>{});
}

int main() {
  int i = 1, j = 2, k = 3;
  // no deduction guides used, we want a ref_wrapper here.
  std::tuple<int, std::reference_wrapper<int>, int> tuple{i, std::ref(j), k};
  auto more = push_back(tuple, 4); // oops, just made a copy of j!
  // dangling reference to the temporary copy made inside push_back
  int& dangling = std::get<1>(push_back(tuple, 4));
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants