Design a Multidimensional Array in C++

Question

I want to design a C++ class for multidimensional arrays. By multidimensional I mean 1d, 2d, 3d, etc. The class supports element by element addition and scalar multiplication operators. Assume A and B are instances of the class (of the same size & dimension). I'd like to use the objects in expressions like this:

C = A * 2 + B

The thing I'm wondered about is how to manage the memory. In the above expression, A * 2 will create a temporary object of the class which is later added to B. Anyways, the temporary object is garbage after the addition is done. I wrote the following class and it works nicely but I am pretty sure it leaks the memory. Now my questions are,

1. How can I fix the memory problem? What is the best way to design the class?

2. Is that possible to allocate the required memory on the stack instead of the heap?

   class XArray
   {
       int num_dim;
       int *dims;
       int *index_helper;
       int table_size;
       double *table;
   
   
   public:
       XArray(const int n, const int *d):num_dim(n), dims(d)
       {
           int size = 1;
           for (int i = 0; i < n; i++) {
               size *= d[i];
           }
           table_size = size;
           table = new double[size];
           index_helper = new int[n];
       };
   
       ~XArray()
       {
           delete[] table;
           delete[] index_helper;
       };
   
       int dim(int d)
       {
           return dims[d];
       }
   
       double& operator()(int i)
       {
           index_helper[0] = i;
           return get_helper(1, index_helper);
       }
   
       double& operator()(int i, int j)
       {
           index_helper[0] = i;
           index_helper[1] = j;
           return get_helper(2, index_helper);
       }
   
       double& operator()(int i, int j, int k)
       {
           index_helper[0] = i;
           index_helper[1] = j;
           index_helper[2] = k;
           return get_helper(3, index_helper);
       }
   
       XArray operator*(double m)
       {
           XArray *xa = new XArray(num_dim, dims);
           for (int i = 0; i < table_size; i++) {
               xa->table[i] = this->table[i] * m;
           }
   
           return *xa;
       }
   
       XArray operator+(const XArray &that)
       {
           if (num_dim != that.num_dim) {
               char *msg = new char[100];
               sprintf(msg, "XArray::dimensions do not match in + operation, expected %d, found %d", num_dim, that.num_dim);
               throw msg;
           }
   
           for (int i = 0; i < num_dim; i++) {
               if (this->dims[i] != that.dims[i]) {
                   char *msg = new char[100];
                   sprintf(msg, "XArray::dimension %d not mached, %d != %d", i, dims[i], that.dims[i]);
                   throw msg;
               }
           }       
   
           XArray *xa = new XArray(num_dim, dims);
           for (int i = 0; i < table_size; i++) {
               xa->table[i] = this->table[i] + that.table[i];
           }
   
           return *xa;
       }
   
   private:
       double& get_helper(int n, int *indices)
       {
           if (n != num_dim) {
               char *msg = new char[100];
               sprintf(msg, "XArray::dimensions do not match, expected %d, found %d", num_dim, n);
               throw msg;
           }
   
           int multiplier = 1;
           int index = 0;
   
           for (int i = 0; i < n; i++) {
               if (indices[i] < 0 || indices[i] >= dims[i]) {
                   char *msg = new char[100];
                   sprintf(msg, "XArray::index %d out of range, %d not in (0, %d)", i, indices[i], dims[i]);
                   throw msg;
               }
   
               index += indices[i] * multiplier;
               multiplier *= dims[i];
           }
   
       return table[index];
   }
   

   };

Answer 1

This question should probably be in Code Review rather than here. But some things on the design:

It is usually not a good idea to manage memory manually, you should prefer using existing containers instead of dynamically allocated arrays. The fact that the constructor acquires ownership of the passed in pointer is not common, and might lead to problems with user code. Currently you are disallowing some uses, like:

int dims[3] = { 3, 4, 5 };
XArray array( 3, dims ); // or use sizeof(dims)/sizeof(dims[0]), or other trickery

Which is simpler to user code than:

int ndims = 5;
int *dims = new int[ndims]
XArray array( ndims, dims );

Which also means that the user must be aware that dims ownership has been acquired and that they cannot delete the pointer.

Internally I would use std::vector to maintain the dynamic memory, as that will give you the proper semantics for free. As it is you need to implement copy constructor and assignment operator (or disable them) as the current code will not leak, but might try to double free some of the memory. Remember the rule of the three: if you provide any of copy constructor, assignment operator or destructor, you should probably provide all three.

Your operator* and operator+ leak memory (the XArray object itself as the internal memory is actually handled by the absence of the copy constructor, but don't think that this is a reason not to create the copy constructor, on the contrary you must create it)

Throwing char* is allowed, but I would recommend that you don't for different reasons. The type of the exception should give information as to what happened, else your user code will have to parse the contents of the exception to determine what went wrong. Even if you decide to use a single type, note that the same rules that are used in overload resolution do not apply in exception handling, and in particular you might have problems with conversions not taking place. This is also another potential for a memory leak in your design, as in throwing a pointer to dynamically allocated memory you delegating the responsibility of releasing the memory to your users, if the user does not release the memory, or if the user does not really care about the exception and just catches everything ( catch (...) {} ) you will leak the errors.

It might be better if instead of throwing you asserted, if that is possible in your program (that is a design decision: how bad is the wrong size, something that can happen even if exceptional, or something that shouldn't happen?).

The member index_helper does not belong to the class, it is an implementation detail that is shared only by the different operator() and the get_helper() method, you should remove it from the class, and you can allocate it statically in each operator(). It is also a bit strange that you offer separate operator() for 1, 2 and 3 dimensions, while the code is generic to handle any dimensions, and also that you are actually requiring the user to know that out of the public interface, two of the operations are forbidden, which is not a great design either.

Answer 2

I would suggest following to avoid memory leaks:

1. Make copy constructor and operator = () private and unimplemented to avoid table and index_helper getting overwritten (and cause memory leak accidently). If you wish you to use them, then make sure, you do delete[] table and delete[] index_helper; before re-assignment to avoid leak.
2. For error messages use std::string instead of char *msg = new char[100];. It's more cleaner and maintainable.
3. Don't use XArray *xa = new XArray(num_dim, dims);; instead simply declare it as XArray xa;. It will be on stack and wound up automatically (if you have c++0x support then you can opt for perfect forwarding to eliminate unnecessary copies)

Now with regards to your remaining class design, the answer will be very subjective and need detail code analysis. So, I will leave it up to you to decide.

Answer 3

I miss copy constructors and assignment operators. You will need these to make copies of the internal buffers and not just copy the pointers or otherwise you will end up freeing the same memory twice.

If you are going to support c++0x you also might want to implement the move semantics for optimal performance.

Throwing dynamically allocated objects like you are doing is a very bad idea.

In your operator* you should create the object on the heap:

XArray operator*(double m)
{
    XArray xa(num_dim, dims);
    for (int i = 0; i < table_size; i++) {
        xa->table[i] = this->table[i] * m;
    }

    return xa;
}

However as long as you don't write your (move) copy constructors this will crash. Because the return statement will make a copy that refers to the same internal data as xa. xa will be destroyed and it's destructor will destroy it's internal data. So the temp object that is being returned points internally to destroyed data.

edit:

Link to info about Move semantics or rvalue references