3

Original Post (with errors)

I want to wrap a call to std::thread constructor (to keep track of all threads running so I can join them or do other things). In this example, the t1 thread gets constructed properly, but the t2 thread doesn't using gcc 4.8.1. However, on Windows (VS2012) it compiles without error and runs without error. Based on the discussion here, this may appear to be a bug in gcc, but arguably it is actually a bug in VS. What is the correct way of doing this?

#include <iostream>
#include <thread>

class A {
public:
    void foo(int n ) { std::cout << n << std::endl; }
};

template<class F, class Arg>
std::thread& wrapper(F&& f, Arg&& a)
{
   std::thread* t = new std::thread(f,a,100);
   return *t;
}

int main()
{
    A a;

    std::thread t1(&A::foo, &a, 100);
    t1.join();

    std::thread t2 = wrapper(&A::foo, &a);
    t2.join();

    return 0;
}

Here is the compiler error

-bash-4.1$ make
g++ -std=c++11    main.cpp   -o main
main.cpp: In function ‘int main()’:
main.cpp:23:41: error: use of deleted function ‘std::thread::thread(std::thread&)’
     std::thread t2 = wrapper(&A::foo, &a);
                                         ^
In file included from main.cpp:2:0:
/opt/rh/devtoolset-2/root/usr/include/c++/4.8.1/thread:125:5: error: declared here
     thread(thread&) = delete;
     ^
make: *** [all] Error 1

Update

I asked the wrong question here and am about to delete it, but because the answers are helpful, I will leave it. The problem was that the Intel icpc 14.0 compiler (not gcc 4.8.1) was throwing the same error regarding bind as discussed here. And it has nothing to do with the "wrapper" but just calling std::thread with a member function instead of static function.

The complaints about the memory leak are 100% valid, but that was me making an unfortunate simplification to the example (from my real code). The actual code saves the threads in a container and deletes the threads on destruction.

Here is a better example:

#include <iostream>
#include <thread>

class A {
public:
    void foo() { }

    template<class Function, class Arg>
    std::thread* wrapper(Function&& f, Arg&& a)
    {
        auto t = new std::thread(f,a);
        return t;
    }
};

int main()
{
    A a;

    std::thread t1(&A::foo, &a);
    t1.join();

    std::thread* t2 = a.wrapper(&A::foo, &a);
    t2->join();
    delete t2;

    return 0;
}

And the output for g++ (4.8.1) which works

-bash-4.1$ make CXX=g++
g++ -lpthread -std=c++11    main.cpp   -o main

and the output for the Intel compiler icpc (14.0) which doesn't work

-bash-4.1$ make CXX=icpc
icpc -lpthread -std=c++11    main.cpp   -o main
/opt/rh/devtoolset-2/root/usr/include/c++/4.8.1/functional(1697): error: class "std::result_of<std::_Mem_fn<void (A::*)()> (A *)>" has no member "type"
        typedef typename result_of<_Callable(_Args...)>::type result_type;
                                                         ^
          detected during:
            instantiation of class "std::_Bind_simple<_Callable (_Args...)> [with _Callable=std::_Mem_fn<void (A::*)()>, _Args=<A *>]" at line 1753
            instantiation of "std::_Bind_simple_helper<_Callable, _Args...>::__type std::__bind_simple(_Callable &&, _Args &&...) [with _Callable=void (A::*)(), _Args=<A *>]" at line 137 of "/opt/rh/devtoolset-2/root/usr/include/c++/4.8.1/thread"
            instantiation of "std::thread::thread(_Callable &&, _Args &&...) [with _Callable=void (A::*)(), _Args=<A *>]" at line 20 of "main.cpp"

/opt/rh/devtoolset-2/root/usr/include/c++/4.8.1/functional(1726): error: class "std::result_of<std::_Mem_fn<void (A::*)()> (A *)>" has no member "type"
          typename result_of<_Callable(_Args...)>::type
                                                   ^
          detected during:
            instantiation of class "std::_Bind_simple<_Callable (_Args...)> [with _Callable=std::_Mem_fn<void (A::*)()>, _Args=<A *>]" at line 1753
            instantiation of "std::_Bind_simple_helper<_Callable, _Args...>::__type std::__bind_simple(_Callable &&, _Args &&...) [with _Callable=void (A::*)(), _Args=<A *>]" at line 137 of "/opt/rh/devtoolset-2/root/usr/include/c++/4.8.1/thread"
            instantiation of "std::thread::thread(_Callable &&, _Args &&...) [with _Callable=void (A::*)(), _Args=<A *>]" at line 20 of "main.cpp"

compilation aborted for main.cpp (code 2)
make: *** [all] Error 2
-bash-4.1$
Community
  • 1
  • 1
Mark Lakata
  • 19,989
  • 5
  • 106
  • 123
  • 6
    That's a terrible wrapper. Why do you allocate objects dynamically?! – Kerrek SB May 08 '14 at 00:55
  • 2
    Your Current Wrapper is also leaking memory. You are calling `new` to create the thread object but you are not calling `delete` to dealloc the memory. – Alex Zywicki May 08 '14 at 01:07
  • 1
    No, this one is definitely a bug in Visual Studio, and is 100% unrelated to the link you provided, which has to do with `std::ref`, wheras your question is about thread copy constructors. – Mooing Duck May 08 '14 at 01:27
  • @KerrekSB -agreed it is a terrible wrapper. It was a bad example, but the problem is not the memory leak, but that I can't get std::thread to construct with `icpc` as shown. – Mark Lakata May 08 '14 at 16:50

2 Answers2

5

Your wrapper should be like this:

template<class F, class Arg>
std::thread wrapper(F&& f, Arg&& a)
{
    return std::thread(std::forward<F>(f), std::forward<Arg>(a));
}
Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
1

std::thread is not copyable. Given that wrapper returns a std::thread&, wrapper(&A::foo, &a); is an lvalue, and thus the initialisation of t2 requires a copy. That's what the compiler error is about.

Sadly, some versions of Visual Studio will happily perform a move here, even though there are no rvalues involved.

Something like std::thread* t = new std::thread(f,a,100); return *t; is pretty much the same as return *new std::thread(f,a,100);, and that clearly shows the memory leak operator: *new. Don't do this.

While not copyable, std::thread is movable. You just have to make it so that wrapper(&A::foo, &a); is an rvalue. That can be done simply by writing the function in the most natural style, like so:

template<class F, class Arg>
std::thread wrapper(F&& f, Arg&& a)
{
    return std::thread(f, a, 100);
}

This however fails to correctly forward the arguments preserving their value category (as is, it turns rvalues into lvalues), even though it's not a problem for this particular example. In order to have general applicability, the function body should forward the arguments properly, like so: return std::thread(std::forward<F>(f), std::forward<Arg>(a), 100);.

Community
  • 1
  • 1
R. Martinho Fernandes
  • 228,013
  • 71
  • 433
  • 510