CODEWITHSUNDEEP
c++multithreadingloops
Tomáš Hodek
Tomáš Hodek

Reputation: 113

C++ Creating threads in loop and store them in vector

I am working with threads for the first time in C++ and I would like to know what is wrong with my code.

I am working on Boruvka`s algorithm and I would like to make threads for finding the shortest Edge for component.

Here is my code: 

    std::vector<std::thread> threads;
    std::vector<Edge> minEdges;

    for (auto g: components) {
        Edge minEd;
        minEdges.push_back(minEd);
        threads.push_back(std::thread (findPath, g, std::ref(minEdges.back())));
    }
    for (auto &i : threads) {
        i.join();
    }
    for (Edge edge:minEdges) {
        if (!contains(mst, edge)) {
            addEdge(&mst, edge.n1, edge.n2, edge.weight);
        }
    }

void findPath(Graph &component, Edge &edge) 
//finds cheapest edge

For exlanation, I would like to give the component and a reference to an element in minEdges where will be stored the cheapest edge. Then I want to join all the threads and put all minimum edges to the mst.

This code gives me an error on line, where I`m pushing threads to the vector, but I could not find why. So can you please tell me?

Error message(i just deleted paths to the files):

  In instantiation of 'struct std::_Bind_simple<void (*(Graph, std::reference_wrapper<Edge>))(Graph&, Edge&)>':
/usr/lib/gcc/x86_64-pc-cygwin/5.4.0/include/c++/thread:142:59:   
required from 'std::thread::thread(_Callable&&, _Args&& ...) [with _Callable = void (&)(Graph&, Edge&); _Args = {Graph&, std::reference_wrapper<Edge>}]'
 graph.cpp:232:82:   required from here
/usr/lib/gcc/x86_64-pc-cygwin/5.4.0/include/c++/functional:1505:61: error: no type named 'type' in 'class std::result_of<void (*(Graph, std::reference_wrapper<Edge>))(Graph&, Edge&)>'
   typedef typename result_of<_Callable(_Args...)>::type result_type;
/usr/lib/gcc/x86_64-pc-cygwin/5.4.0/include/c++/functional:1526:9: error: no type named 'type' in 'class std::result_of<void (*(Graph, std::reference_wrapper<Edge>))(Graph&, Edge&)>'
     _M_invoke(_Index_tuple<_Indices...>)
     ^
                                                         ^

Thank you for the answers.

Upvotes: 2

Views: 2756

Answers (1)

rustyx
rustyx

Reputation: 85541

A few points...

  1. Don't modify vectors which are already in use by other threads (adding to a vector can move other elements)

  2. Iterate over references, not temporaries (auto g)

  3. Prefer emplace_back whenever possible

  4. Optionally try the lambda syntax

  5. Use synchronization to be sure things happen in the order you think they would (not needed in your case - thread creation acts as a memory fence)

    std::vector<Edge> minEdges;
    for (auto &g: components) {
        minEdges.emplace_back();
    }

    std::vector<std::thread> threads;
    for (size_t i=0; i<components.size(); ++i) {
        threads.emplace_back([&]{findPath(components[i], minEdges[i]);});
    }
    // rest of code...

Upvotes: 1

Related Questions