Gfrktyl

I wrote a updater_thread() and a reader_thread(), and used lock_guard to protect a global instance.

Question

1. I've never used lock_guard. In this code, lock_guard is used properly?




2. Is this code thread-safe?

Code

#include <iostream>

#include <thread>

#include <mutex>

#include <iterator>

#include <algorithm>

#include <vector>

#include <initializer_list>

#include <unistd.h>



class C {

  std::vector<int> v;

  std::mutex mutex_;



public:

  C() {}

  void Update(const std::vector<int> &new_v) {

    std::lock_guard<std::mutex> lock(mutex_);

    v = new_v;

  }

  bool Check(const int x){

    std::lock_guard<std::mutex> lock(mutex_);

    return std::find(v.begin(), v.end(), x) != v.end();

  }

  /* dump() is not essential */

  void dump() {

    std::lock_guard<std::mutex> lock(mutex_);

    std::cout << "dump: ";

    std::copy(v.begin(), v.end(),

              std::ostream_iterator<int>(std::cout, " "));

    std::cout << "n";

  }

};



// create an instance gloablly

C g_c;



void updater_thread() {

  std::cout << "start updater_threadn";

  while (true) {

    std::vector<int> v;

    for (int i = 0; i < 10; i++) {

        v.push_back(rand() % 10 + 1);

    }

    std::sort(v.begin(), v.end());

    g_c.Update(v);

    std::cout << "updated!!!n";

    std::copy(v.begin(), v.end(),

              std::ostream_iterator<int>(std::cout, " "));

    std::cout << "n";

    sleep(5);

  }

}



void reader_thread() {

  std::vector<int> v {1,2,3,5};

  while (true) {

    std::cout << "check: non-exist item: ";

    for (int i = 0; i < v.size(); i++) {

        if (!g_c.Check(v[i])){

            std::cout <<  v[i] << " ";

        }

    }

    std::cout << "n";

    sleep(1);

  }

}



int main() {

  std::thread t_up(updater_thread);

  std::thread t_r(reader_thread);

  t_up.join();

  t_r.join();

}

Yes, that all looks correct to me!

You don't use anything from <unistd.h> (non-standard) or <initializer_list> (obscure), so I recommend removing those includes.

You sigil the mutex_ data member with an underscore, but you don't sigil the v member. I recommend being consistent:

std::mutex mutex_;

std::vector<int> v_;

Personally I would spell the member's name mtx_, but that's just a personal habit; I don't know if that naming convention is widespread. (cv_ for a condition variable certainly is, though!)

Consider that your Check and dump methods don't need to mutate the object, so they should be declared const. This means that your mutex_ data member will need to be declared mutable so that you can still lock and unlock it inside your const member functions.

Also, consider picking a capitalization rule and sticking to it. Why Check and Update but dump (not Dump)?

Check's parameter x is marked const but that marking serves no purpose: eliminate it. (Const is a contract.)

for (int i = 0; i < v.size(); i++) {

    if (!g_c.Check(v[i])){

        std::cout <<  v[i] << " ";

    }

}

This could be rewritten more concisely as

for (int vi : v) {

    if (!g_c.Check(vi)) {

        std::cout << vi << " ";

    }

}

Your multithreading stuff all looks great!