Skip to main content
Code Review

Return to Question

Tweeted twitter.com/StackCodeReview/status/817537660914896896
Rollback to Revision 1
Source Link
Mathieu Guindon
Mathieu Guindon
  • 75.1k
  • 18
  • 191
  • 463

EDIT

After all of the valuable comments I have modified my class. The one thing. I've tried to incorporate all of the comments and also my findings online. The one missing piece left is described by @David problem with destruction of an empty Buffer, but I'll update the solution once this will be fixed.

#include <deque>
#include <thread>
#include <mutex>
#include <condition_variable>

template <typename T>
class Buffer
{
public:
    void add(const T& num)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.push_back(num);
        locker.unlock();
        cond.notify_one();
    }
    void add(T&& num)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.push_back(std::move(num));
        locker.unlock();
        cond.notify_one();
    }
    template<typename ...Args>
    void emplace_back(Args&&... args)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.emplace_back(std::forward<Args>(args)...);
        locker.unlock();
        cond.notify_one();
    }
    T remove()
    {
        std::unique_lock<std::mutex> locker(mu);
        cond.wait(locker, [this](){return !buffer.empty();});
        T front(std::move(buffer.front()));
        buffer.pop_front();
        return front;
    }
    int size()
    {
        std::lock_guard<std::mutex> lock{mu};
        return buffer.size();
    }
private:
    std::mutex mu;
    std::condition_variable cond;

    std::deque<T> buffer;
};

EDIT

After all of the valuable comments I have modified my class. The one thing. I've tried to incorporate all of the comments and also my findings online. The one missing piece left is described by @David problem with destruction of an empty Buffer, but I'll update the solution once this will be fixed.

#include <deque>
#include <thread>
#include <mutex>
#include <condition_variable>

template <typename T>
class Buffer
{
public:
    void add(const T& num)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.push_back(num);
        locker.unlock();
        cond.notify_one();
    }
    void add(T&& num)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.push_back(std::move(num));
        locker.unlock();
        cond.notify_one();
    }
    template<typename ...Args>
    void emplace_back(Args&&... args)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.emplace_back(std::forward<Args>(args)...);
        locker.unlock();
        cond.notify_one();
    }
    T remove()
    {
        std::unique_lock<std::mutex> locker(mu);
        cond.wait(locker, [this](){return !buffer.empty();});
        T front(std::move(buffer.front()));
        buffer.pop_front();
        return front;
    }
    int size()
    {
        std::lock_guard<std::mutex> lock{mu};
        return buffer.size();
    }
private:
    std::mutex mu;
    std::condition_variable cond;

    std::deque<T> buffer;
};
fixed class sourcecode
Source Link
sebap123
sebap123
  • 237
  • 2
  • 7

EDIT

After all of the valuable comments I have modified my class. The one thing. I've tried to incorporate all of the comments and also my findings online. The one missing piece left is described by @David problem with destruction of an empty Buffer, but I'll update the solution once this will be fixed.

#include <deque>
#include <thread>
#include <mutex>
#include <condition_variable>

template <typename T>
class Buffer
{
public:
    void add(const T& num)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.push_back(num);
        locker.unlock();
        cond.notify_one();
    }
    void add(T&& num)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.push_back(std::move(num));
        locker.unlock();
        cond.notify_one();
    }
    template<typename ...Args>
    void emplace_back(Args&&... args)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.emplace_back(std::forward<Args>(args)...);
        locker.unlock();
        cond.notify_one();
    }
    T remove()
    {
        std::unique_lock<std::mutex> locker(mu);
        cond.wait(locker, [this](){return !buffer.empty();});
        T front(std::move(buffer.front()));
        buffer.pop_front();
        return front;
    }
    int size()
    {
        std::lock_guard<std::mutex> lock{mu};
        return buffer.size();
    }
private:
    std::mutex mu;
    std::condition_variable cond;

    std::deque<T> buffer;
};

EDIT

After all of the valuable comments I have modified my class. The one thing. I've tried to incorporate all of the comments and also my findings online. The one missing piece left is described by @David problem with destruction of an empty Buffer, but I'll update the solution once this will be fixed.

#include <deque>
#include <thread>
#include <mutex>
#include <condition_variable>

template <typename T>
class Buffer
{
public:
    void add(const T& num)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.push_back(num);
        locker.unlock();
        cond.notify_one();
    }
    void add(T&& num)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.push_back(std::move(num));
        locker.unlock();
        cond.notify_one();
    }
    template<typename ...Args>
    void emplace_back(Args&&... args)
    {
        std::unique_lock<std::mutex> locker(mu);
        buffer.emplace_back(std::forward<Args>(args)...);
        locker.unlock();
        cond.notify_one();
    }
    T remove()
    {
        std::unique_lock<std::mutex> locker(mu);
        cond.wait(locker, [this](){return !buffer.empty();});
        T front(std::move(buffer.front()));
        buffer.pop_front();
        return front;
    }
    int size()
    {
        std::lock_guard<std::mutex> lock{mu};
        return buffer.size();
    }
private:
    std::mutex mu;
    std::condition_variable cond;

    std::deque<T> buffer;
};
Source Link
sebap123
sebap123
  • 237
  • 2
  • 7

Multi-thread safe buffer in C++

I am trying to design thread-safe data structure that I cna use as a buffer in my application. Can you please give me comments about this code and what can be improved:

#include <deque>
#include <thread>
#include <mutex>
#include <condition_variable>

template <typename T>
class Buffer
{
public:
    void add(T num)
    {
        while (true)
        {
            std::unique_lock<std::mutex> locker(mu);
            buffer.push_back(num);
            locker.unlock();
            cond.notify_all();
            return;
        }
    }
    T remove()
    {
        while (true)
        {
            std::unique_lock<std::mutex> locker(mu);
            cond.wait(locker, [this](){return buffer.size() > 0;});
            T back = buffer.back();
            buffer.pop_back();
            locker.unlock();
            cond.notify_all();
            return back;
        }
    }
    int size()
    {
        std::unique_lock<std::mutex> locker(mu);
        int s = buffer.size();
        locker.unlock();
        return s;
    }
private:
    std::mutex mu;
    std::condition_variable cond;

    std::deque<T> buffer;
};