Skip to main content
added 498 characters in body
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180

Don't declare Iterator in the global namespace

While the name Deque is quite unique, what if you also want to implement your own Vector, Array and Map? These would all need their own iterator types, so you will then run into an issue if you have the genericly named Iterator in the global namespace. There are several solutions:

  • Rename Iterator to DequeIterator.
  • Create a unique namespace where you can put this Iterator in.
  • Move Iterator into class Deque.

Don't declare Iterator in the global namespace

While the name Deque is quite unique, what if you also want to implement your own Vector, Array and Map? These would all need their own iterator types, so you will then run into an issue if you have the genericly named Iterator in the global namespace. There are several solutions:

  • Rename Iterator to DequeIterator.
  • Create a unique namespace where you can put this Iterator in.
  • Move Iterator into class Deque.
Source Link
G. Sliepen
  • 69.3k
  • 3
  • 75
  • 180

It leaks memory when used as a queue

The name "deque" stands for double-ended queue. A typical way to use a deque is when you need a queue where you push only into one end and pop only from the other end. Often, the total number of elements in a deque has a small upper bound. However, in this scenario, your Deque will allocate more and more memory the longer it is used, as it never deletes blocks until the whole Deque object is destructed.

Use more helper classes

It's already great that you used std::vector for front_blocks and back_blocks, that saves you a lot of trouble having to manage memory for those. But you can create some more classes yourself to simplify Deque itself. For example, you could create a class Block that represents one block, with helper functions to construct, access and destroy its elements.

Use std::unique_ptr to avoid managing memory for the blocks.

Also consider creating a class Index that groups a block_idx and idx_in_block together. You can then for example add operator++() and operator--() member functions to make modying an index as simple as possible.

Use std::size_t for counts and indices

You use int for counts and indices, but an int might not be large enough to handle all the elements you could store in memory. Instead, use std::size_t. Of course, if you really need to handle negative indices, you need something else, like std::ptrdiff_t.

Also, consider that a single std::size_t is large enough to be able to index any amount of elements that could ever be stored in memory. So instead of having a "block index" and "index in block", you can have a single index where the low bits are the index inside the block, and the high bits the block index.

Reduce the size of an Iterator

Your Iterator stores two pointers and three integers. That's quite a lot. You can simplify this by storing just a pointer to the Deque object itself, and you can store the index in a single integer; I've already mentioned that above, and it seems deque_idx already fulfills that role.

The reduced size will help a lot in case a user wants to store many iterators somewhere. And while it looks less efficient to dereference a Deque* to get a pointer to front_blocks and/or back_blocks, the code will be inlined anyway, and the compiler will very likely be able to optimize that away.

Make BLOCK_SIZE a static member

Instead of having a global constant BLOCK_SIZE, make it a static constexpr member variable. This way, you can make it have a different value depending on sizeof(T). Consider for example having a Deque<char>: allocating only 16 bytes for a block is very inefficient, while for Deque<HugeClass> you might even want to have a BLOCK_SIZE of 1.

Missing const versions of front() and back()

You added const iterators and several other const member functions, but you forgot to add const versions of front() and back().

Unnecessary swap()

Your swap() just calls swap() on all member variables. In that case, you don't need to implement swap() at all.