Skip to main content
added 1 character in body
Source Link
Incomputable
  • 9.7k
  • 3
  • 34
  • 73
  • Asymmetry: the call to set does bounds checking, whereas get does not.

  • No check for success of allocation, if new in constructor failed, you're in realrealm of undefined behavior.

  • Everything vaguely related to array indices should be std::size_t, to guarantee that it will be possible to access furthest elements. It will also imply to the programmers that negative values are prohibited.

  • <algorithm> and <iterator> are needed as a consequence of strange seenIndex. I find it great for the library class to have as few dependencies as possible.

  • Looping to copy contents of one vector to another can be replaced by a very trivial std::memset(). It might be faster as well.

  • May be it is about design, but get() not returning a reference is rather odd.

  • Asymmetry: the call to set does bounds checking, whereas get does not.

  • No check for success of allocation, if new in constructor failed, you're in real of undefined behavior.

  • Everything vaguely related to array indices should be std::size_t, to guarantee that it will be possible to access furthest elements. It will also imply to the programmers that negative values are prohibited.

  • <algorithm> and <iterator> are needed as a consequence of strange seenIndex. I find it great for the library class to have as few dependencies as possible.

  • Looping to copy contents of one vector to another can be replaced by a very trivial std::memset(). It might be faster as well.

  • May be it is about design, but get() not returning a reference is rather odd.

  • Asymmetry: the call to set does bounds checking, whereas get does not.

  • No check for success of allocation, if new in constructor failed, you're in realm of undefined behavior.

  • Everything vaguely related to array indices should be std::size_t, to guarantee that it will be possible to access furthest elements. It will also imply to the programmers that negative values are prohibited.

  • <algorithm> and <iterator> are needed as a consequence of strange seenIndex. I find it great for the library class to have as few dependencies as possible.

  • Looping to copy contents of one vector to another can be replaced by a very trivial std::memset(). It might be faster as well.

  • May be it is about design, but get() not returning a reference is rather odd.

Source Link
Incomputable
  • 9.7k
  • 3
  • 34
  • 73

I'll list the things you should know to fully comprehend the review:

1.Automatic storage (sometimes called stack) and free store (heap)

2.Exceptions

3.Sad story about new and operator new

4.Data layout in memory, the thing that C++ standard says about it

Code Review:

I will not consider small and nitpicking optimizations, but I will provide some basic ones. I'll add another interface at the bottom of the post, since this one, may be not worst, but still is not good enough.

Anomaly:

I've got confused by the member variables of the class. Most people usually use something like this:

int* arr;
std::size_t size;
std::size_t capacity;

and are done with it.No need for anything else. In fact, you can decrease this thing further, leaving only a pointer (to do that, people usually "prefix" the size and capacity). Even after some answers on my questions I couldn't get why seenIndex should be there.

std::size_t:

Why std::size_t(aka size_t from C) ? The reason is that C++ (and probably C) standard guarantees that std::size_t will be large enough to index the whole, theoretically possible memory. std::numeric_limits<std::size_t>::max() will give you theoretical maximum memory available on the platform, but not the memory installed in the machine.

new:

In my opinion, new is a fail. It is one of the worst fails in the history of C++. The new should be an allocation mechanism, not a factory, which it is, currently. So, even if others will disagree, I will advice using std::malloc(). The cast will look ugly, but it is the thing that is called at the end of the day. All major compilers (VC++, icc, g++, clang++) call malloc from operator new. Array version of new is even worse, since it requires it to calldelete[], when there is no technical reason to make people call it.

this->member:

Personally I don't like this way of accessing members.

Since the bug in the pushfront() has been mentioned I will omit it.

pushback():

new might throw std::bad_alloc() exception. This will cause the pushback to terminate without doing the thing it should. But it is not the real problem here. The real problem is that you modify the size before checking if the new allocation is succeeded. So, what will happen is that size will increase, but the array on the heap will not. I will leave the possible consequences to your imagination.

Miscellaneous:

  • Asymmetry: the call to set does bounds checking, whereas get does not.

  • No check for success of allocation, if new in constructor failed, you're in real of undefined behavior.

  • Everything vaguely related to array indices should be std::size_t, to guarantee that it will be possible to access furthest elements. It will also imply to the programmers that negative values are prohibited.

  • <algorithm> and <iterator> are needed as a consequence of strange seenIndex. I find it great for the library class to have as few dependencies as possible.

  • Looping to copy contents of one vector to another can be replaced by a very trivial std::memset(). It might be faster as well.

  • May be it is about design, but get() not returning a reference is rather odd.

Better design:

The first thing I found ridiculous was that I can't get the size of the container, so I can't iterate over it. This is why you needed the print() function, since you don't know the size of the container. I recommend the interface of std::vector<>. May be it is not the best, but it is very close to perfection.