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

FWD implementation #1

Closed
bitwizeshift opened this issue Sep 22, 2016 · 4 comments
Closed

FWD implementation #1

bitwizeshift opened this issue Sep 22, 2016 · 4 comments

Comments

@bitwizeshift
Copy link

First of all I wanted to say impressive talk today at CPPCon for static_if and static_for -- excellent ideas and implementations. During the talk though I noticed something that might be worth bringing up.

In the implementation of the FWD macro there are a couple of possible errors

Currently it's written as:

#define VRM_CORE_FWD(...) ::std::forward<decltype(__VA_ARGS__)>(__VA_ARGS__)

Unless this std::forward is a custom implementation, there are two things here that may lead to more cryptic errors.

  1. the template type<decltype(__VA_ARGS__)> will only deduce the type of the last arg in the set, because it would expand with the comma operator. For example, decltype(1,1.0) only deduces double and not int,double.
  2. std::forward in the standard only takes one argument, making the __VA_ARGS__ part of this likely unnecessary.

The reason I bring this up is because something like FWD(1,1.0) will essentially translate into a call of:
::std::forward<double>( 1, 1.0 ) -- and this leads to strange compiler messages.
It may be worth changing to just FWD(x) instead.

@vittorioromeo
Copy link
Owner

Really glad you liked the talk - thanks for the feedback and for bringing this issue up. The reason I implemented FWD with __VA_ARGS__ instead of a single parameter is because having just one parameter behaves poorly with templated types:

#define COOL_MACRO(x) do_something(x)
COOL_MACRO(std::pair<int, float>{1, 5.f});

Now that I think about it, I don't believe this problem applies to FWD as:

  • The template parameter T passed by the user to std::forward<T>(x) has to be the type of a forwarding reference, and there's no way a comma can appear there.
  • The value x passed to std::forward<T>(x) has to be the name of the forwarding reference, and there's no way a comma can appear there. Passing a temporary (e.g. std::forward<...>(std::pair<int, int>{0, 1}) does not make sense).

Unless there is a possible situation I am missing where either T or x contain an "extraneous" comma then your proposed changes are beneficial. Can you think of any situation where a comma may be part of a well formed std::forward call? If not, I will probably change __VA_ARGS__ to a single argument.

@bitwizeshift
Copy link
Author

bitwizeshift commented Sep 22, 2016

The __VA_ARGS__ for composite template types makes sense; I hadn't actually considered that issue. That's one of many problems when working with macros.

Can you think of any situation where a comma may be part of a well formed std::forward call?

There may be a few cases where it could be valid:

  • calling a generic function and forwarding the return type (e.g. FWD( func(1,2) )), where func is from a template.
  • Abusing the comma-operator in some weird way (e.g. FWD( (doX(), y) ); // calls doX(), then forwards y
  • As you mentioned, passing in temporaries

One other approach that should work seamlessly without any breakage is to write a c++14 wrapper around std::forward that uses trailing-return decltype syntax:

template<typename T>
constexpr auto FWD(T&& x) -> decltype(std::forward<T>(x)){ return std::forward<T>(x); }

Assuming this library isn't trying to keep any conformance to c++11, this should be valid and retain the normal semantics of std::forward (since it deduces the return type), while not requiring the template type in the argument. The benefit is that this would give clearer failures if used improperly than the macro-based approach.

It's just a thought either way.

Once again, great library and great work 👍

@vittorioromeo
Copy link
Owner

vittorioromeo commented Sep 22, 2016

I don't think that will work correctly. See this basic example and this StackOverflow question. (Also, isn't your FWD conforming to C++11 anyway?)

@bitwizeshift
Copy link
Author

Whoops, didn't actually realize that caused issues -- thanks for the link! Not sure why I thought that would work...

As for the c++14 conformance, that was more a comment since constexpr was added to std::forward in c++14, but was not present in c++11 (iirc).

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

No branches or pull requests

2 participants