2
\$\begingroup\$

The Original Post (v1)

I'm looking for feedback on the updated version of the code posted at the link above.

#pragma once

#include <memory>
#include <cassert>
#include <type_traits>

namespace datastructures {

    template<class _Ty, size_t _Size>
    class CircularBufferv2 {

        // uses char as type to prevent default _Ty initialization.
        alignas(alignof(_Ty)) char buffer[_Size * sizeof(_Ty)];
        size_t head;
        size_t tail;
        bool isFull;

    public:
        constexpr CircularBufferv2() noexcept :
            buffer{0},
            head{0},
            tail{0},
            isFull{false} {
        }

        void push(const _Ty& item) noexcept {
            assert(!isFull && "Attempting to push item into full buffer!");

            new (&buffer[head * sizeof(_Ty)]) _Ty(std::move(item));

            head = ++head % _Size;
            isFull = head == tail;
        }

        _Ty pop() noexcept {
            assert(!is_empty() && "Attempting to pop item from empty buffer!");

            auto location = reinterpret_cast<_Ty*>(&buffer[tail * sizeof(_Ty)]);
            auto result = std::move(*location);
            std::destroy_at(location);

            tail = ++tail % _Size;
            isFull = false;

            return result;
        }

        _NODISCARD constexpr _Ty& peek() noexcept {
            assert(!is_empty() && "Attempting to peek in empty buffer!");

            return *reinterpret_cast<_Ty*>(&buffer[tail * sizeof(_Ty)]);
        }

        _NODISCARD constexpr const _Ty& peek() const noexcept {
            assert(!is_empty() && "Attempting to peek in empty buffer!");

            return *reinterpret_cast<_Ty*>(&buffer[tail * sizeof(_Ty)]);
        }

        _NODISCARD constexpr bool is_empty() const noexcept {
            return !isFull && tail == head;
        }

        _NODISCARD constexpr size_t get_capacity() const noexcept {
            return _Size;
        }

        _NODISCARD constexpr size_t get_size() const noexcept {
            if (isFull)
                return _Size;

            if (head >= tail)
                return head - tail;

            return _Size + head - tail;
        }

        _NODISCARD _CONSTEXPR17 _Ty* data() noexcept {
            return buffer;
        }

        _NODISCARD _CONSTEXPR17 const _Ty* data() const noexcept {
            return buffer;
        }
    };
}

I want to take advantage of all of the new features(c++17) while also supporting older compilers(preferably all older compilers but c++11 is probably as old as i'll actually be compiling for). Any suggestions welcome. (I'm attempting to use this class as an example class to follow when constructing other classes.)

Also, the use of _CONSTEXPR17 on the data functions I was wondering why use the macro vs just a constexpr? (I based the use of macros around the std::array struct, its data function uses _CONSTEXPR17 rather than just constexpr.)

\$\endgroup\$
  • 2
    \$\begingroup\$ You are aiming for C++11/C++17 compilers, yet use non-standard macros. Why? Why not stick to the standard? \$\endgroup\$ – Cris Luengo May 22 at 2:31
  • 2
    \$\begingroup\$ Identifiers starting with underscore followed by a capitol letter are reserved for the implementation. i.e. They are not designed for use by application developers and thus you should not be using them. In this context _Ty, _Size, _CONTEXPR17, _NODISCARD should not be used in your code. \$\endgroup\$ – Martin York May 22 at 6:17
  • 1
    \$\begingroup\$ What are the rules about using an underscore in a C++ identifier? \$\endgroup\$ – Martin York May 22 at 6:19
  • \$\begingroup\$ You probably shouldn’t be using v2 in the class name anyway. Consider using inline namespaces if you need different versions. \$\endgroup\$ – L. F. May 22 at 6:48
  • \$\begingroup\$ @L.F. v2 will be removed once i finalize the class so you can ignore that. \$\endgroup\$ – Hex May 22 at 6:50
3
\$\begingroup\$

Instead of a char array you can use std::aligned_storage.

You aren't consistent with your macros you have both constexpr and _CONSTEXPR17. Pick one and stick to it.

destroy_at is standard only from C++17 onward. The equivalent syntax is location->~_Ty(); However it may be worth to put this and the placement new into a helper function to move the ugly syntax elsewhere and clarify the function.

You don't have a move variant of push or a bulk add/remove using iterators.

\$\endgroup\$
1
\$\begingroup\$

Is there any reason for your users to access your internal buffer? To me that data methods shouldn't be there.

Don't like very much those reinterpret_cast but I see you have those there to support not default constructable types. I could live with those and get back that additional feature as long as that implementation detail is sealed into the class.

\$\endgroup\$
  • \$\begingroup\$ What do you mean by "I could live with those and get back that additional feature as long as that implementation detail is sealed into the class."? \$\endgroup\$ – Hex May 22 at 6:55
  • 1
    \$\begingroup\$ Right now you are giving access to external users to your implementation details. Your users need to know how to get Ty from the internal buffer. That is not a nice thing if the access is not that straight forward. \$\endgroup\$ – Giuseppe Puoti May 22 at 7:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.