HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

(Optionally Concurrent) FIFO

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
concurrentoptionallyfifo

Problem

Based on Concurrent FIFO in C++11 and my review I implemented a queue and its concurrent pendant.

Is there anything left to improve regarding clarity, usability, code-style, lock-times or general efficiency?

```
#ifndef FIFO_H
#define FIFO_H

#include
#include
#include
#include
#include

template
class ST_FIFO
{
static_assert(CAPACITY, "Needs to have non-zero capacity");
T data[CAPACITY + 1];
std::size_t input_index = 0;
std::size_t output_index = 0;

inline static constexpr std::size_t wrap_index(std::size_t index) noexcept
{ return index > CAPACITY ? index - CAPACITY - 1 : index; }
public:
static constexpr std::size_t capacity() noexcept { return CAPACITY; }

bool empty() const noexcept { return input_index == output_index; }

std::size_t size() const noexcept
{
return input_index >= output_index
? input_index - output_index
: input_index + CAPACITY + 1 - output_index;
}

template
auto push(X&& x) noexcept(noexcept(pop(data), data = std::forward(x)))
-> decltype(*data = std::forward(x), true)
{
if(size() == CAPACITY)
pop(data[input_index]);
data[input_index] = std::forward(x);
input_index = wrap_index(input_index + 1);
return true;
}

template
auto try_push(X&& x) noexcept(noexcept(*data = std::forward(x)))
-> decltype(*data = std::forward(x), true)
{
if(size() == CAPACITY)
return false;
data[input_index] = std::forward(x);
input_index = wrap_index(input_index + 1);
return true;
}

std::size_t multi_push(const T ts[], size_t count)
noexcept(noexcept(push(*ts)))
{
for (size_t i = 0; i = size())
return false;
t = data[wrap_index(output_index + ind)];
return true;
}
};

template
class MT_FIFO : ST_FIFO
{
std::atomic_bool wait_flag = true;
mutable std::mutex mutex;
mutable std::condition_variabl

Solution

Your popped elements will not be destructed. You move them but that isn't the same as being destructed and it doesn't guarantee that memory will be released.

Furthermore your container requires T to be default constructible which prevents immutable objects from being used with the queue.

See this question for how to resolve both: Implementation of fixed size queue using a ring (cyclic) buffer.

Your push function is not exception safe.

I dislike all capital names as typically all capital identifiers are used for macros.

The name FIFO is poor in my opinion because the concept you're modelling is a queue or a pipe (these are similar but different). First in first out is just a description of how data enters and leaves the container. FIFO doesn't allude to the fact that it will overwrite on push if the container is full which is a bit surprising if you're not familiar with the container. Personally I do not like this behaviour as it violates the principle of least surprise.

Also I'm not a fan of the ST_ prefix which I assume means single threaded... If you're adding that prefix to this class, you should add it to all classes which are not concurrent safe, which quickly becomes obnoxious. As for the MT_ prefix, that is acceptable but as it is an attribute of the queue, I'd rather see it as a suffix. I would prefer naming the class something like concurrent_fixed_pipe.

Context

StackExchange Code Review Q#163400, answer score: 8

Revisions (0)

No revisions yet.