Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

make unary_function/binary_function optional (C++11) #624

Closed
andrewcorrigan opened this issue Jan 23, 2015 · 18 comments
Closed

make unary_function/binary_function optional (C++11) #624

andrewcorrigan opened this issue Jan 23, 2015 · 18 comments

Comments

@andrewcorrigan
Copy link
Contributor

This is intended for the C++11 milestone:

Can requiring that unary_function and binary_function be the base class for a UnaryFunction or BinaryFunction in Thrust algorithms be made optional?

Not only does this remove the need for deriving from these types, I think (I'm just now starting to really learn C++11) that it also removes the need for writing constructors for small functors since they can instead be instantiated with aggregate initialization once the functors would no longer have a base class.

struct adds_a : public thrust::unary_function<int,int>
{
  int _a;
  adds_a(int a) : _a(a) {}
  int operator()(int i) const { return i + this->_a; }
};
...
adds_a(3);

becomes:

struct adds_a
{
  int _a;
  int operator()(int i) const { return i + this->_a; }
};
...
adds_a{3};

I've already made this change in my private copy of Thrust via std::result_of in thrust/detail/type_traits/result_of.h.

template<typename Signature>
  struct result_of
{
  typedef typename std::result_of<Signature>::type type;
};
@jaredhoberock jaredhoberock added this to the C++11 milestone Jan 23, 2015
@andrewcorrigan
Copy link
Contributor Author

Would you be interested in pull requests that can non-invasively start making use of C++11?

#if __cplusplus >= 201103L    // or something like THRUST_USE_CXX11?
template<typename Signature>
  struct result_of
{
  typedef typename std::result_of<Signature>::type type;
};
#else
template<typename Signature, typename Enable = void> struct result_of;

// specialization for unary invocations of things which have result_type
template<typename Functor, typename Arg1>
  struct result_of<
    Functor(Arg1),
    typename thrust::detail::enable_if<thrust::detail::has_result_type<Functor>::value>::type
  >
{
  typedef typename Functor::result_type type;
}; // end result_of
#endif
// specialization for binary invocations of things which have result_type
template<typename Functor, typename Arg1, typename Arg2>
  struct result_of<
    Functor(Arg1,Arg2),
    typename thrust::detail::enable_if<thrust::detail::has_result_type<Functor>::value>::type
  >
{
  typedef typename Functor::result_type type;
};
#endif

@schiller-manuel
Copy link

@jaredhoberock How is your plan to continue the development Thrust with C+11 support?

@schiller-manuel
Copy link

This would really help when using templated functors in combination with zip iterators, e.g:

template <typename Tuple>
struct Fun : public thrust::unary_function<Tuple, Tuple>
{
    __host__ __device__
    Tuple operator()(Tuple p)
    {
        return p;
    }
};

thrust::device_vector<int> v1(10);
thrust::device_vector<int> v2(10);
auto zip_begin = thrust::make_zip_iterator(thrust::make_tuple(v1.begin(), v2.begin()));

typedef ... TupleType; // what is the exact type of the tuple? 
auto transform_it_begin = thrust::make_transform_iterator(zip_begin, Fun<TupleType>());

If thrust::unary_function was not needed, I could simply write:

struct Fun
{
    template <typename Tuple>
    __host__ __device__
    Tuple operator()(Tuple p)
    {
        return p;
    }
};

// ...
auto transform_it_begin = thrust::make_transform_iterator(zip_begin, Fun());

@jaredhoberock
Copy link
Contributor

If you guys want to work out a pull request for this one, I'll review and merge.

@schiller-manuel
Copy link

@jaredhoberock Would you like to have a global #define switch like THRUST_USE_CXX11 or just use __cplusplus >= 201103L ? This does not only apply here but is a general decision on how to handle c++11 features.

@jaredhoberock
Copy link
Contributor

I'd prefer not introducing new switches. I assume if people are already opting into something like -std=c++11 through the compiler, they want to use C++11 throughout their project.

@schiller-manuel
Copy link

Would c++11 support render thrust/detail/type_traits.h unnecessary and we could simply use <type_traits>?

@jaredhoberock
Copy link
Contributor

There's a lot of non-standard stuff in thrust/detail/type_traits.h, so it needs to remain. If I understand @andrewcorrigan's earlier post, this feature shouldn't require anything beyond result_of, right?

@schiller-manuel
Copy link

Yes, it only uses std::result_of.

@schiller-manuel
Copy link

regarding __cplusplus:

I don't use Visual Studio, but it seems like that at least up to VS2013, __cplusplus is still 199711L (source). This means code using Thrust which relies on C++11 features being enabled through that macro won't compile in VS.

@jaredhoberock
Copy link
Contributor

That's fine, I wouldn't want to rely on C++11 features unless that macro reported they were available.

@schiller-manuel
Copy link

Yet, these C++11 features are available on VS. In my platform independent project, I still have to derive from thrust::unary_function because my code will not compile otherwise in VS because this macro is still set erroneously to 199711L by VS.

I know it looks ugly, but how about using the aforementioned preprocessor switch THRUST_USE_CXX11 to enable Thrust's C++11 features instead of relying on __cplusplus >= 201103L? This switch can be automatically set when the compiler reports the correct version:

#ifndef THRUST_USE_CXX11
    #if __cplusplus >= 201103L
        #define  THRUST_USE_CXX11
   #endif
#endif

I could then globally define in my code:

#define THRUST_USE_CXX11

This would change nothing for GCC, but would enable Thrust's C++11 features for VS.

@jaredhoberock
Copy link
Contributor

That's fine -- we needn't engage in heroics to enable this feature. VS2013 users can just use thrust::unary_function and upgrade to a newer compiler that reports that it supports C++11.

@schiller-manuel
Copy link

@jaredhoberock I updated my comment

@jaredhoberock
Copy link
Contributor

Thrust supports more compilers than just gcc and the Microsoft compiler. It's very expensive to track down which language features are available on which versions of the different compilers. Explicitly defining a hypothetical THRUST_USE_CXX11 could not enable support where it does not exist.

Suppose a compiler supported C++11 feature A, but not B and therefore sets a conservative value for __cplusplus. If a user were to instruct Thrust to ignore __cplusplus by globally setting THRUST_USE_CXX11, the program would fail when Thrust attempted to use feature B.

There is already a standard means to introspect whether language features are available by using the __cplusplus preprocessor symbol. It's much easier to communicate to users which features of Thrust are available by describing their support in terms of this standard facility rather than an ad hoc set of macros which pollute the global namespace and create the potential for ODR violations.

@schiller-manuel
Copy link

I definitely see your point and agree.

@jaredhoberock
Copy link
Contributor

There may be a compromise. I would make an exception for the feature test macros recommended by SD6 because they are a near standard and Thrust would not have to create its own preprocessor symbols to introspect.

If there is an appropriate feature test macro for std::result_of, Thrust could check that if a compiler reports a insufficient __cplusplus.

@schiller-manuel
Copy link

There is one, but then again, we need a solution to detect c++11 for other features as well (see variadic tuple).

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

No branches or pull requests

3 participants