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

Support for reference_wrappers in make_xxx #176

Open
ldionne opened this issue Sep 8, 2015 · 9 comments
Open

Support for reference_wrappers in make_xxx #176

ldionne opened this issue Sep 8, 2015 · 9 comments
Labels

Comments

@ldionne
Copy link
Member

ldionne commented Sep 8, 2015

Right now, containers are documented as not supporting references at all. First, this should be changed because most containers are able to hold references. Furthermore, it would be nice to add support for reference_wrapper in make_tuple, like std::make_tuple. Specifically, hana::make_tuple(std::ref(x)) should create a hana::tuple<X&> instead of a hana::tuple<std::reference_wrapper<X>>.

Things I'm worried about:

  • Compile-time cost of adding even more machinery to make_tuple. This will have to be benchmarked to be sure.
  • Compile-time cost of including <functional> in such a fundamental header. This has to be benchmarked too, and we can probably cheat with a forward declaration otherwise.

As a motivating example for this feature, consider the following:

struct Person {
    std::string name;
};

auto xs = hana::make_tuple(Person{"Bob"}, Person{"John"});
auto ys = hana::transform(xs, [](Person& p) {
    return std::ref(p.name);
});

// Right now, ys contains reference_wrappers, which is hard to manipulate.
// This feature would give us the desired intuitive behavior.

Containers that should probably support this feature if we decide to go forward:

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

As for basic_tuple, I think it is better to keep it as basic as possible for performance reasons. I don't think anybody using basic_tuple would expect that kind of sugar anyway.

@ldionne ldionne added the feature label Sep 8, 2015
@ldionne
Copy link
Member Author

ldionne commented Sep 13, 2015

After benchmarking the compile-time cost of adding support for this,

  • Adding additional machinery to make_tuple is fine.
  • Including <functional> just to get the forward declaration of std::reference_wrapper is not fine. It adds about 0.2s of compile-time when just including hana/tuple.hpp. Also, note that we can't forward declare stuff in the std:: namespace, so we can't just forward declare std::reference_wrapper ourselves. And this is not only a nitpick; for example, libc++ defines its stuff in the std::__1 inline namespace, so it really does not work to forward declare in std::.

This is really lame; all we want is a <functional_fwd> header, but we really can't take the hit to include the whole <functional> header, especially since that header is not likely to be included by users on their own anyway, unlike <type_traits> for example. I see two solutions:

  1. We forward declare std::reference_wrapper in the std:: namespace, and make sure it works on libc++ and libstdc++. For this, we will need to keep the forward declarations in sync with the standard libraries. This is hacky.
  2. We declare our own hana::reference_wrapper, which stupidly reinvents the wheel and forces users to use our own instead of std's. This is also unsatisfactory.

@ldionne
Copy link
Member Author

ldionne commented Sep 13, 2015

This is really lame; all we want is a <functional_fwd> header, but we really can't take the hit to include the whole <functional> header

@ericniebler, I saw that you also forward declare stuff in namespace std manually in your Meta library. Do you think it would be worth it to try and add forward declaration headers to the standard library? Do you think that might ever pass on the standard committee? It seems like this would be useful.

@badair
Copy link
Contributor

badair commented Nov 19, 2015

I see two solutions: ...

Perhaps there is a third (still far from ideal):

What if we allow the client to pass a reference_wrapper-like template as a template template arg? This is not ideal for a number of reasons, and I'm not well-versed in the performance implications of this (might be really bad, idk), but at a glance it seems like this might strike a balance between performance and usability. It would certainly add some unfortunate complexity to the code.

Sure, nobody really wants to pass std::reference_wrapper as a template arg, but nobody wants to use a hana::reference_wrapper shim or wait for a standardized forward declaration either.

Is that a plausible solution?

@badair
Copy link
Contributor

badair commented Nov 19, 2015

Edit: I just realized there is an open PR for this. I'll check it out later (I'm on mobile so I can't easily edit my above comment).

@ldionne
Copy link
Member Author

ldionne commented Nov 19, 2015

@badair I think this is not necessarily a bad solution. However, this issue is stalled because of the problems documented in #179 (comment) more than those documented above.

@badair
Copy link
Contributor

badair commented Feb 2, 2016

What are your thoughts on wrapping <functional> dependent code with an #ifdef, for those who cannot or wish not to include <functional>?

@ldionne
Copy link
Member Author

ldionne commented Feb 2, 2016

I think it is not necessary to take this burden upon ourselves. <functional> is a standard header, and it is provided by all implementations I know of. My opinion is that we should strive not to include <functional> in the library for compile-time performance reasons, but if we decide to do so (e.g. because we have no other choice), then I think users should have no control over it. Defining a macro just for this would be adding an additional customization point, thus making the library more complex to use and maintain, for little benefit.

@ericniebler
Copy link
Member

@ldionne +1

@FrankHB
Copy link

FrankHB commented Jul 23, 2018

I meet exactly the same issue about cost of inclusion of <functional> in my own library (and I know forward declaration is a no-go because it is not conforming). The worse thing is now std::invoke_wrapper in <type_traits> is required to handle std::reference_wrapper correctly and I am simulating that feature in my library, it is unwise to use header names different to the standard which requires additional work for compatibility. I wonder whether it is considered by the committee...

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

No branches or pull requests

4 participants