8
\$\begingroup\$

I was in need of a vector-like type that could hold types that couldn't be copied or moved so I implemented one. I tried to make it similar to standard STL containers.

Is the use of the macro to throw the exception for the at member function legitimate to prevent code repetition between the const and non const version of at?

I didn't include all the code as there is a lot of repetitions for getters, and I don't have support for allocators yet.

Do you see design or implementation flaws?

Any advice to improve it?

#define FIXED_BUFF_THROW_OUT_RANGE(pos, size) (throw std::out_of_range("out of range : " + std::to_string(pos) + " >= " + std::to_string(size) + " in " + std::string(__PRETTY_FUNCTION__)))
//pretty function is a clang/gcc extension similar to __func__ be with a full function signature

template<typename T>
class fixed_buffer {
 public:
  using value_type = T;
  using size_type = std::size_t;
  using difference_type = std::ptrdiff_t;
  using reference = value_type&;
  using const_reference = const value_type&;
  using iterator = value_type*;
  using const_iterator = const value_type*;
 private:
  static constexpr bool is_noexcept_destructible = noexcept(std::declval<value_type>().~value_type());
 private:
  iterator _begin;
  iterator _end;
  iterator _alloc;
 public:
  fixed_buffer() noexcept {
    _begin = nullptr;
    _end = nullptr;
    _alloc = nullptr;
  }
  explicit fixed_buffer(size_type size) {
    _begin = static_cast<iterator>(std::aligned_alloc(alignof(value_type), sizeof(value_type) * size));
    _end = _begin;
    _alloc = _begin + size;
  }
  fixed_buffer(const fixed_buffer&) = delete;
  fixed_buffer operator =(const fixed_buffer&) = delete;
  fixed_buffer(fixed_buffer&& other) noexcept {
    _begin = other._begin;
    _end = other._end;
    _alloc = other._alloc;
    other._begin = nullptr;
    other._end = nullptr;
    other._alloc = nullptr;
  }
  fixed_buffer& operator= (fixed_buffer&& other) noexcept(is_noexcept_destructible) {
     if (_begin) {
       clear();
       free(_begin);
     }
    _begin = other._begin;
    _end = other._end;
    _alloc = other._alloc;
    other._begin = nullptr;
    other._end = nullptr;
    other._alloc = nullptr;
    return *this;
  }
  reference operator [](size_type pos) {
    return _begin[pos];
  }
  const_reference operator [](size_type pos) const {
    return _begin[pos];
  }
  reference at(size_type pos) {
    if (pos < size())
      return this->operator[](pos);
    FIXED_BUFF_THROW_OUT_RANGE(pos, size());
  }
  const_reference at(size_type pos) const {
    if (pos < size())
      return this->operator[](pos);
    FIXED_BUFF_THROW_OUT_RANGE(pos, size());
  }
  template<typename ... Ts>
  void remplace_at(size_type pos, Ts&& ...ts) {
    _begin[pos].~value_type();
    new(_begin + pos) value_type(std::forward<Ts>(ts)...);
  }
  template<typename ... Ts>
  void emplace(Ts&& ... ts) {
    new(_end++) value_type(std::forward<Ts>(ts)...);
  }
  void pop_back() {
    back().~value_type();
    _end--;
  }
  void clear()  noexcept(is_noexcept_destructible) {
    for (iterator it = _begin; it != _end; it++) {
      it->~value_type();
    }
    _end = _begin;
  }
  void swap(fixed_buffer& other)  noexcept(is_noexcept_destructible) {
    fixed_buffer tmp = std::move(other);
    other = std::move(*this);
    *this = std::move(tmp);
  }
  size_type size() const noexcept {
    return _end - _begin;
  }
  size_type capacity() const noexcept {
    return _alloc - _begin;
  }
  iterator begin() noexcept {
    return _begin;
  }
  const_iterator begin() const noexcept {
    return _begin;
  }
  iterator end() noexcept {
    return _end;
  }
  const_iterator end() const noexcept {
    return _end;
  }
  reference front() noexcept {
    return *_begin;
  }
  const_reference front() const noexcept {
    return *_begin;
  }
  reference back() noexcept {
    return _end[-1];
  }
  const_reference back() const noexcept {
    return _end[-1];
  }
  ~fixed_buffer()  noexcept(noexcept(std::declval<value_type>().~value_type())) {
    if (_begin) {
      clear();
      free(_begin);
    }
  }
};

#undef FIXED_BUFF_THROW_OUT_RANGE
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Some member functions might be missing (at least size() is referred to inside at, but no declaration of it is to be found). Please check and add the missing functions. \$\endgroup\$
    – hoffmale
    Aug 16, 2018 at 1:22
  • \$\begingroup\$ @hoffmale i added the function that was missing. i removed them initially because they were taking a lot of space but i thought they weren't relevant. \$\endgroup\$
    – Tyker
    Aug 16, 2018 at 8:10

2 Answers 2

7
\$\begingroup\$

Your code is quite good, with some improvement margins:

Be more expressive

Uninitialized memory management isn't the best-known or the most often used area of C++. It does even look arcane to many C++ programmers. What I suggest is to make it more accessible:

  1. for "placement destructor call", there already is a standard function: std::destroy_at, which is arguably more readable than pointer->~Type(). And there's a range version (std::destroy) that would fit very well in your destructor.

  2. for placement new, you could write a mirror function, construct_at.

The your remplace_at would become very clear:

std::destroy_at(_begin+pos);
construct_at (_begin+pos, std::forward<Args>(args)...);

I would also advise against negative array subscription, like return _end[-1] which, although correct, is unsettling. More generally, you should be consistent in the use of your iterators: use either subscription or pointer-like syntax. My personal taste would be:

// front()
return *_begin; // or better: return *begin() once you've defined it
// back()
return *std::prev(_end);

Be more concise

There's a lot of code repetition you could avoid.

You can now initialize your class variables where you declare them, so there is no need to do it in your default constructor, which can remain default:

 private:
  iterator _begin = nullptr;
  iterator _end   = nullptr;
  iterator _alloc = nullptr;

You can use swap to define your assignment operator and your value constructor, instead of doing it the other way around:

fixed_buffer(fixed_buffer&& other) noexcept {
    swap(other, *this);
}

// ...

fixed_buffer& operator= (fixed_buffer&& other) noexcept(is_noexcept_destructible) {
    // other is correctly destroyed at the end of this function, 
    // taking care of previous *this resources
    auto tmp = std::move(other);
    swap(tmp, *this); 
    return *this;
 }

You could have your pointers inside a struct to make it trivially swappable -you wouldn't even have to write swapthen.

Macros should disappear from your code. If you want to avoid code duplication, write a function that computes the right message and pass its results to the exception's constructor:

std::string out_of_range_message(std::size_t pos, std::size_t size, const std::string& function_name);

// ...
// inside `at`
throw std::out_of_range(out_of_range_message(pos, size, __PRETTY_FUNCTION__));
\$\endgroup\$
4
  • 1
    \$\begingroup\$ // other is correctly destroyed at the end of this function, No, it won't be in all cases: fixed_buffer a{}, b{}; and somwhere else a = std::move(b); Now b is still alive, keeping the old content from a alive (and b might be at a global scope, thus doing so for much longer than anticipated). A better implementation of the move assignment would be auto temp = std::move(other); swap(temp); return *this; \$\endgroup\$
    – hoffmale
    Aug 16, 2018 at 10:33
  • \$\begingroup\$ @hoffmale: you're right, fixed! \$\endgroup\$
    – papagaga
    Aug 16, 2018 at 10:41
  • 1
    \$\begingroup\$ Since I'm too lazy to write an answer for this tidbit, you might want to add that remplace_at and emplace should do some bounds checking ;) \$\endgroup\$
    – hoffmale
    Aug 16, 2018 at 10:45
  • 1
    \$\begingroup\$ And I'm too lazy to write an answer to say we normally prefer to use initializers rather than assignment of members in our constructors. \$\endgroup\$ Aug 16, 2018 at 10:52
1
\$\begingroup\$
  static constexpr bool is_noexcept_destructible = 
      noexcept(std::declval<value_type>().~value_type());

Did you want std::is[_nothrow]_destructible?


  iterator _begin;
  iterator _end;
  iterator _alloc;

public:
  fixed_buffer() noexcept {
    _begin = nullptr;
    _end   = nullptr;
    _alloc = nullptr;
  }

Prefer the member initializer list over assignment in the constructor body.

  fixed_buffer() noexcept 
      : _begin{nullptr}
      , _end{nullptr}
      , _alloc{nullptr} {
  }

When you are initializing with constants (0, 3.14, nullptr, ...), prefer in-class default member initializers.

  iterator _begin = nullptr;
  iterator _end   = nullptr;
  iterator _alloc = nullptr;

public:
  fixed_buffer() = default; // nothing to do at construction, just =default.

  explicit fixed_buffer(size_type size) 
      : _begin{reserve_initially(size)}
      , _end{_begin};
      , _alloc{_begin + size} {}

protected:
  iterator reserve_initially(size_type n) {
    return static_cast<iterator>(
        std::aligned_alloc(alignof(value_type), sizeof(value_type) * size));
  }

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

If T is copyable, should fixed_buffer also be copyable?


  template<typename ... Ts>
  void remplace_at(size_type pos, Ts&& ...ts) {

replace_at? How about a replace that takes an iterator? Consider returning a reference to your newly constructed object for the appropriate modifying operations.

    _begin[pos].~value_type();

Like with your at function, consider bounds checking here. There is also std::destroy_at.


  template<typename ... Ts>
  void emplace(Ts&& ... ts) {
    new(_end++) value_type(std::forward<Ts>(ts)...);
  }

If you are providing a vector-like interface, consider naming this emplace_back. Consider including a bound checked version.

  template <typename... Ts>
  reference emplace_back_capped(Ts&&... ts) {
    if (_end != _alloc) {
      /* ... */
    }
    throw /* ... */
  }

  void clear()  noexcept(is_noexcept_destructible) {
    for (iterator it = _begin; it != _end; it++) {
      it->~value_type();
    }
    _end = _begin;
  }

The loop is std::destroy.


      free(_begin);

Make sure you are calling the C++ version (std::free). The C++ standard makes no guarantee that the unqualified C version will exist.


Implement operations that may make sense:

  • reserve_capped - Should fixed_vector be reservable if empty?
  • resize/resize_down - Resizing up not possible, down is pop_back() n times.
  • shrink_to_fit - Doesn't need to copy or even deallocate, but resize the capacity to wherever _end is.
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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