# Producer-Consumer in C++ - follow-up

To expand from the changes supplied by the answer of my previous question:

#include <condition_variable>
#include <iostream>
#include <random>
#include <mutex>
#include <vector>

// global variables
std::condition_variable cv;
std::mutex mtx;
std::vector<char> data;

int count = 0, buff_size = 0;

char random_char() {

return static_cast<char>(dist(generator));
}

/* Consumer

Prints out the contents of the shared buffer.

*/
void consume() {
std::unique_lock<std::mutex> lck(mtx);

while (count == 0) {
cv.wait(lck);
}

for (const auto& it : data) {
std::cout << it << std::endl;
}

count--;
}

/* Producer

Randomly generates capital letters in the range of A to Z,
prints out those letters in lowercase, and then
inserts them into the shared buffer.

*/
void produce() {
std::unique_lock<std::mutex> lck(mtx);

char c = random_char();
std::cout << " " << static_cast<char>(tolower(c)) << std::endl;
data.push_back(c);

count++;
cv.notify_one();
}

int main() {
std::cout << "The Producer-Consumer Problem (in C++11!)" << std::endl << "Enter the buffer size: ";
std::cin >> buff_size;

// keep the buffer in-range of the alphabet
if (buff_size < 0) {
buff_size = 0;
}
else if (buff_size > 26) {
buff_size = 26;
}

// initialize the arrays
for (int order = 0; order < buff_size; order++) {
}

for (int order = 0; order < buff_size; order++) {
processed[order].join();
production[order].join();
}

std::cout << "Succeeded with a shared buffer of " << data.size() << " letters!";
}


Global variables in the best of cases are questionable, but in this case especially you have buff_size as a global variable. buff_size is only used in main() so should be a local variable there.

You can also take advantage of the fact that std::condition_variable::wait() has another overload that takes a predicate to replace:

while (count == 0) {
cv.wait(lck);
}


with:

cv.wait(lck, []{ return count > 0; });


Although, did you really want every consumer to print out every letter every time? If buff_size were 10, you'd end up logging 55 letters. Was that intentional? If it wasn't, you could drop count entirely, switch data to be a std::queue and have your consumer be:

std::unique_lock<std::mutex> lck(mtx);
cv.wait(lck, []{return !data.empty(); });
std::cout << data.front() << std::endl;
data.pop();


On the producer front, did you really mean notify_one() and not notify_all()? Let all the consumers fight for the lock!

Lastly, this:

if (buff_size < 0) {
buff_size = 0;
}
else if (buff_size > 26) {
buff_size = 26;
}


is a clamp. It might be worth writing something like:

template <typename T>
T clamp(T const& val, T const& lo, T const& hi)
{
return std::min(std::max(val, lo), hi);
}

buff_size = clamp(buff_size, 0, 26);

• Wouldn't their be some draw back to letting all of the consumers fight each other? What's the benefit of allowing it? – T145 Sep 21 '15 at 16:15
• What about if I wanted the buffer to filled completely before the consumer begins working on it? – T145 Sep 21 '15 at 19:14
• @T145 The drawback of all consumers fighting each other is possibility of Starvation. [ en.wikipedia.org/wiki/Starvation_(computer_science) ] This would occur if one consumer could not get its desired resource. Letting all consumers wait while buffer is not full is extremely inefficient, producer might be slow, certain consumers (in practice) might not even need whole produce. – kushj Sep 22 '15 at 2:45