Why std::move is not working with std::list

Question

I am unable to compile the below program.

void toSin(std::list<double>&& list)
{
    std::for_each(list.begin(), list.end(), [](double& x)
    {
        x = sin(x);
    });
}

int main()
{
    std::list<double> list;
    const double pi = 3.141592;
    const double epsilon = 0.0000001;
    for (double x = 0.0; x < 2 * pi + epsilon; x = x + pi / 16)
    {
        list.push_back(x);
    }
    // Start thread
    std::thread th(toSin, std::move(list));
    th.join();
    return 0;
}

I get > error C2664: 'void (std::list<double,std::allocator<_Ty>> &&)' : cannot convert argument 1 from 'std::list<double,std::allocator<_Ty>>' to 'std::list<double,std::allocator<_Ty>> &&'

Reproduce I cannot. Which version of visual studio are you using? Mind you, I added in a bunch of missing headers. — user4581301, Jul 04 '17 at 17:32
The line `std::thread th(toSin, std::move(list));` implies that you should not iterator over `list` beyond that point, since it's moved away. But you try to iterate over it on the very next line. — François Andrieux, Jul 04 '17 at 17:33
2013 is missing a lot of C++ 11 support. Compiles in 2015, but shouldn't run the way you want. See if you can upgrade. Also heed @FrançoisAndrieux's warning. — user4581301, Jul 04 '17 at 17:35
You probably want **l-value** reference instead of r-value reference for `toSin`, and then `std::thread th(toSin, std::ref(list));` — Jarod42, Jul 04 '17 at 17:36
Thanks for pointing out @ François Andrieux. However, this code works perfectly fine — NARESH KITTUR, Jul 04 '17 at 17:38
In additoin to FrançoisAndrieux comment, If you expect to use same list, you will have UB as concurrent access is not synchronized (mutex, ...). — Jarod42, Jul 04 '17 at 17:39
This code compiles in VS2013. However, after executing the line test(std:: move(list));, I could still see the size of the list is 1. void test(std::list && list) { } int main() { std::list list; test(std::move(list)); } — NARESH KITTUR, Jul 04 '17 at 17:45
The code compiles with gcc 5.4.0. With `g++ -Wall -std=c++11 main.cpp -lpthread` gives no errors and no warnings after I included the appropriate header files. — snow_abstraction, Jul 04 '17 at 18:05
Looks like you have a case where the compiler sees [better advantage in not `move`ing](http://ideone.com/lIhxm0), but with a [more complicated case based on your question...](http://ideone.com/rjWOWg) In other words don't count on it. [Assume nothing about the state of the moved from other than you can destroy it](https://stackoverflow.com/questions/20850196/what-lasts-after-using-stdmove-c11). — user4581301, Jul 04 '17 at 18:10

Curious · Answer 1 · 2017-07-04T17:54:05.850

I feel like your compiler is wrong here. A decayed (copied) value type should be bindable to an rvalue reference.

Anyway take a look at this quote from the documentation

3) Creates new std::thread object and associates it with a thread of execution. The new thread of execution starts executing
std::invoke(decay_copy(std::forward<Function>(f)), decay_copy(std::forward<Args>(args))...);

Basically anything you pass as an argument to the constructor of std::thread will be copied as a function argument to the function.

Also know that your function will work just fine if you make it accept the std::list variable by value instead of by rvalue reference. See Correct usage of rvalue references as parameters for more

If your intent is to pass a reference to a variable to a thread function, the way I do it usually is with a lambda

std::list<double> lst;
auto th = std::thread{[&lst]() {
    toSin(lst);
}};

But you can also use std::ref for the same effect. I just personally feel the lambda approach is clearer.

std::list<double> lst;
auto th = std::thread{toSin, std::ref(lst)};

Also as correctly pointed out in the comments, you have a race condition in your code that you should prevent with a mutex, or wait for the thread to finish

auto th = std::thread{[&lst]() {
    toSin(lst);
}};
th.join();

// then iterate and print out

Updated the code above, I just wanted to have a unique memory to the worker thread to avoid synchronization issues. — NARESH KITTUR, Jul 04 '17 at 17:57
@NARESHKITTUR if you do that however, how do you intend to print out the list later on in the main thread? — Curious, Jul 04 '17 at 17:59
True, I will not be able to use the list in the main thread. I will have to process/print the list in the worker thread itself. So to get around this problem I should be passing the list by reference with the mutex. — NARESH KITTUR, Jul 04 '17 at 18:09
@NARESHKITTUR correct, or wait for the thread to finish via `.join()` — Curious, Jul 04 '17 at 18:15

score 0 · Answer 2 · answered Jul 04 '17 at 18:20

I think that you maybe miss some #include, that code works on Visual Studio 2015

#include <algorithm>
#include <list>
#include <thread>
void toSin(std::list<double>&& list)
{
    std::for_each(list.begin(), list.end(), [](double& x)
    {
        x = sin(x);
    });
}

int main()
{
    std::list<double> list;
    const double pi = 3.141592;
    const double epsilon = 0.0000001;
    for (double x = 0.0; x < 2 * pi + epsilon; x = x + pi / 16)
    {
        list.push_back(x);
    }
    // Start thread
    std::thread th(toSin, std::move(list));
    th.join();
    return 0;
}

Visual Studio 2015 is not Visual Studio 2013. 2013 has gaping holes in its c++11 support. Odds are good that it doesn't compile. I don't have easy access to 2013, couldn't see the point of upgrading until the holes were filled, so I don't have a copy to try it out. — user4581301, Jul 04 '17 at 18:33

Why std::move is not working with std::list

2 Answers2