Ggthjy

I am trying to implement a simple raw queue in C++. This is what I have come up with

#include <queue>

#include <cstdint>

#include <array>

#include <cstring>



class simple_queue

{

private:

    static constexpr uint32_t max_msg_size = 4096;

    using char_msg = std::array<char, max_msg_size>;

    std::queue<char_msg> char_queue;



public:

    simple_queue() = default;

    ~simple_queue() = default;

    simple_queue(simple_queue&&) = default;

    simple_queue& operator=(simple_queue&&) = default;



    uint32_t write(const char* const buff, const uint32_t size) noexcept

    {

        const uint32_t size_to_copy = std::min(max_msg_size, size);

        char_msg tmp_msg;

        std::memcpy(&tmp_msg, buff, size_to_copy);

        char_queue.push(std::move(tmp_msg));

        return size_to_copy;

    }



    uint32_t read(char* const buff, const uint32_t size) noexcept

    {

        if (char_queue.empty())

        {

            return 0;

        }

        const uint32_t size_to_copy = std::min(max_msg_size, size);

        std::memcpy(buff, &char_queue.front(), size_to_copy);

        char_queue.pop();

        return size_to_copy;

    }

};

Note that I am trying to implement this queue just to store raw char buffers. As far as I understand making it a template does not make sense in this case.

I have played with it a bit and it seems to work as I expect.

• What can I change with this implementation?


• Does it make sense to use a custom allocator in here? I am allocating memory every time I write to the queue; how can I use a custom allocator to allocate some default chunk of memory when the queue is constructed?

You are not allocating per se when when writing to the queue. std::array is backed by a plain C-array on the stack, that gets moved.

So you only ever allocate depending on the underlying container the std::queue is based on. By default this is a std::deque.

I do not really understand the need for the std::array in the type though. Why not just use a std::string and limit the size of it to 4096? There is even a constructor that does explicitely that string (const char* s, size_t n)

Before I get to the Code there are some other things I would like to mention:

1. why are you not useing std::copy instead of memcpy. The former works better with C++ and will in the end almost always end up as memcpy?




2. You do not need to define the special member functions. In fact you have forgotten 2 of then, aka copy assignment and copy constructor. Why do I say forgotten? Because there is no way to tell. So If you want your queue to be move only then you should actually delete those special member functions you dont want.




3. In your read function you never check whether the size you want to read is actually valid. Is that intendend? If so why? A std::array is not initialized so the memory in it that is not written by you will be random. You are not writing to it but you are copying it around. So you should actually take the minimum out of size and queue.front().size()




4. std::string has a member function std::string::copy that copyies a certain amount of chars to a buffer (http://www.cplusplus.com/reference/string/string/copy/). I would suggest to use that for writing back to the buffer

That leads me to the following:

#include <string>

#include <queue>

#include <vector>



class simple_queue

{

private:

    static constexpr uint32_t max_msg_size = 4096;

    std::queue<std::string, std::vector<std::string>> char_queue;



public:  

   simple_queue(const simple_queue&) = delete;

   simple_queue& operator=(const simple_queue&) = delete;



    uint32_t write(const char* const buff, const uint32_t size) noexcept

    {

        const uint32_t size_to_copy = std::min(max_msg_size, size);

        char_queue.emplace(buff, size_to_copy);

        return size_to_copy;

    }



    uint32_t read(char* const buff, const uint32_t size) noexcept

    {

        if (char_queue.empty())

        {

            return 0;

        }

        std::string& msg = char_queue.front();

        const uint32_t size_to_copy = std::min(msg, size);

        msg.copy(buff, size_to_copy, 0);



        char_queue.pop();

        return size_to_copy;

    }

};

EDIT:

I forgot to mention, that now you should use a std::vector as backing of the queue as your are not storing a gargantuan array but rather a mall std::string.

Note that it actually ends up being the same. The std::deque based std::array implementation is backed by a linked list, so each array ends up in a node of a linked list, which is kind of similar to the separate allocation of the std::string