Skip to main content
Spelling fixes
Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

You have two namespaces?.
Any particular reason.?

Seems relatively generic. I might expect this yo be alreadyto have already been used. I normally include the namespace as part of the guard macro.

But you do want to mark it as noexcept. This is because you also want to mark the move constructor  / move assignment as noexcept and the easy way to implement these is to call swapswap.

You are constructing TT here. This has a couple of issues. The type T may not have a default constructor. The type T may be very expensive to construct so you don't actually want to construct until you know you are going to use it. Think about when you resize the vector from 100 -> 200 elements. You are now going to construct 100 elements but you may only use 1 of those 100 elements. Do you really want to pay the cost.? When you reduce the size of the vector you currently can notcannot destroy the object but the user may want to force the release of the resources.

On the same sort of vaine.vein:

In this constructor you are constructing sz objects of type T.:

I saw you define iterators.
Why can I not use the range based for-based for loop here.?

What happened to the move assignemntassignment operator?

The reason for marking the move operations as noexcept asis that it allows certain optimizations when your object is placed into container.

In your insertinsert, you copy elements when resizing.

You have two namespaces?
Any particular reason.

Seems relatively generic. I might expect this yo be already have been used. I normally include the namespace as part of the guard macro.

But you do want to mark it as noexcept. This is because you also want to mark the move constructor/ move assignment as noexcept and the easy way to implement these is to call swap.

You are constructing T here. This has a couple of issues. The type T may not have a default constructor. The type T may be very expensive to construct so you don't actually want to construct until you know you are going to use it. Think about when you resize the vector from 100 -> 200 elements. You are now going to construct 100 elements but you may only use 1 of those 100 elements. Do you really want to pay the cost. When you reduce the size of the vector you currently can not destroy the object but the user may want to force the release of the resources.

On the same sort of vaine.

In this constructor you are constructing sz objects of type T.

I saw you define iterators.
Why can I not use the range based for loop here.

What happened to the move assignemnt operator?

The reason for marking the move operations as noexcept as it allows certain optimizations when your object is placed into container.

In your insert you copy elements when resizing.

You have two namespaces.
Any particular reason?

Seems relatively generic. I might expect this to have already been used. I normally include the namespace as part of the guard macro.

But you do want to mark it as noexcept. This is because you also want to mark the move constructor  / move assignment as noexcept and the easy way to implement these is to call swap.

You are constructing T here. This has a couple of issues. The type T may not have a default constructor. The type T may be very expensive to construct so you don't actually want to construct until you know you are going to use it. Think about when you resize the vector from 100 -> 200 elements. You are now going to construct 100 elements but you may only use 1 of those 100 elements. Do you really want to pay the cost? When you reduce the size of the vector you currently cannot destroy the object but the user may want to force the release of the resources.

On the same sort of vein:

In this constructor you are constructing sz objects of type T:

I saw you define iterators.
Why can I not use the range-based for loop here?

What happened to the move assignment operator?

The reason for marking the move operations as noexcept is that it allows certain optimizations when your object is placed into container.

In your insert, you copy elements when resizing.

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

Overview

You have two namespaces?
Any particular reason.

You tried to implement move semantics for the Vector. You missed the move assignment. But you forget to implement move semantics for the T type. So large types in your vector are expensive to use.

You don't provide the strong exception guarantee. You should definitely try. If something throws your object can be left in a bad state.

The standard for this:

// 1: Do anything the can throw on temporary objects.
// 2: Use noexcept safe functions to swap temp with state.
// 3: Cleanup.

A couple of const correctness issues.

Code Review:

Seems relatively generic. I might expect this yo be already have been used. I normally include the namespace as part of the guard macro.

#ifndef VECTOR_H_
#define VECTOR_H_

I would expect swap() to be implemented as a member method but not required. The friend function swap then simply calls the member function.

But you do want to mark it as noexcept. This is because you also want to mark the move constructor/ move assignment as noexcept and the easy way to implement these is to call swap.

        friend void swap( Vector &lhs, Vector &rhs )

Yes. But be careful with this declaration.

            T* memory_block;

When you allocate you don't want to construct.


The problem here:

            Vector() : size_index( 0 ), vector_capacity( 1 ), memory_block ( new T[ vector_capacity ]{} ){}

You are constructing T here. This has a couple of issues. The type T may not have a default constructor. The type T may be very expensive to construct so you don't actually want to construct until you know you are going to use it. Think about when you resize the vector from 100 -> 200 elements. You are now going to construct 100 elements but you may only use 1 of those 100 elements. Do you really want to pay the cost. When you reduce the size of the vector you currently can not destroy the object but the user may want to force the release of the resources.

So in general you want to allocate space but you don't want to construct the object. That is why we keep a capacity and a size as different values.


On the same sort of vaine.

In this constructor you are constructing sz objects of type T.

            Vector( std::size_t sz, const T &val ) :  Vector( sz )

Then the first thing you do is copy over all the elements in the container. So you are basically destroying the old value and re-creating the new values.

            {
                for( std::size_t i = 0; i != vector_capacity; ++i ) {
                    memory_block[ i ] = val;
                    ++size_index;
                }
            }

This means you have effectively done the same work twice. Also you are copying elements that are not valid. Simply copy size not capacity elements.


I saw you define iterators.
Why can I not use the range based for loop here.

                for( std::size_t i = 0; i != vector_capacity; ++i ) {
                    memory_block[ i ] = val;
                    ++size_index;
                }

Yep. But declare the move constructor as noexcept.

            Vector( Vector &&other_vec ) { swap( *this, other_vec ); }

What happened to the move assignemnt operator?

The reason for marking the move operations as noexcept as it allows certain optimizations when your object is placed into container.


Good try. But you potentially leak.

            void push_back( const T &val )
            {

               // STUFF

                    T * new_memory_block = new T[ vector_capacity * 2 ];

                    // What happens if the copy constructor of T throws
                    // an exception during this operation?
                    //
                    // In that case you have leaked `new_memory_block`
                    // and all the resources held by the T objects in this
                    // block.   
                    for( std::size_t i = 0; i != size_index; ++i )
                        new_memory_block[ i ] = memory_block[ i ];

                    // What happens if the destructor throws in a T?
                    // probably an application quits but you don't know.
                    //
                    // If the delete throws and is escapes the destructor
                    // and the excetion is caught higher in the call stack
                    // then you have leaked `new_memory_block` and resouces
                    // this object is in an invlalid state as the memory_block
                    // is no longer valid.
                    delete [] memory_block;

                    // Sure this is safe. :-)
                    memory_block = new_memory_block;
                    vector_capacity *= 2;
                }
                memory_block[ size_index++ ] = val;
            }

A simpler way to implement this:

 void push_back( const T &val )
 {
     if( size_index >= vector_capacity ) {
         Vector<T>  temp(vector_capacity * 2);
         std::move(std::begin(*this), std::end(*this), std::begin(temp));                                                 
         swap(*this, temp);
     }
     memory_block[ size_index++ ] = val;
 }

The push back is fine:

 void push_back(T const& val);

But T could be large. You should allow T to be moved into your vector or constructed in place.

 void push_back(T&& val);
 template<typename... Args)
 void emplace_back(Args&&... args);

In your insert you copy elements when resizing.

            void insert( int index, const T& val )
            {
                    // This is a copy operation not a move operation.
                    // If T is large then this can be costly.
                    memory_block[ temp_index + 1 ] = memory_block[ temp_index ];
                }
            }

            }

Interesting:

            void clear()
            {
                delete [] memory_block;
                memory_block = nullptr;
                size_index = 0;
            }

For const versions of front() and back. I would expect them to return a const reference.

            T& front() const { return memory_block[ 0 ]; }
            T& back() const { return memory_block[ size_index - 1 ]; }