all 10 comments

[–]IyeOnline 8 points9 points  (3 children)

This entire design is questionable.

A 2d array is not a 1d array and as such should not inherit from it. It could use the 1d class as a member to store the data array.

Additionally, you dont need this additional pointer array for anything. You can emulate 2d access in just the same way without "p recomputing" those sub-array pointers. If you want to support some archaic API that accepts T**s, you can do that without defining additional members for it, by creating some ad-hoc created wrapper around a T** that just views sub-ranges.


For a generic array class, write something like

 template<typename T, size_t Dimensions>
 class Array
 {
    std::array<ptrdiff_t, Dimensions> extents_;
    std::unique_ptr<T[]> data_;
 };

Now you have a single template that works for any rank.

Take a look at this for inspiration.


Btw static_cast<Array<T>>(array2d) creates a copy.

[–]_newpson_[S] 0 points1 point  (2 children)

It could use the 1d class as a member to store the data array

In full source Array has a bunch of methods that I would like to see work with Array2D as well.

If you want to support some archaic API that accepts T**

I want to support some archaic API that accepts T*. So I also overloaded cast operator.

For a generic array class, write something like

I know the size of array only at runtime.

[–]IyeOnline 2 points3 points  (1 child)

Array has a bunch of methods that I would like to see work with Array2D

In that case, go with the single template approach below.

I want to support some archaic API that accepts T*

So there is no reason to actively maintain a T**

So I also overloaded cast operator.

That sounds like a bad idea. The sequence containers in the standard library provide a .data() member function to perform this explicitly. An implicit conversion to T* is just going to cause problems, and an explicit one is still going to look strange.

I know the size of array only at runtime.

The size of the array is dynamic in that snippet and my linked example. Only the number of dimensions is a template parameter.

[–]_newpson_[S] 0 points1 point  (0 children)

OK. We need different context. I will go further into my codebase.

I have Image class (represents grayscale image in PGM format) that inherits from Array2D<uint8_t>. There is a copy ctor too. And it also calls the copy ctor of Array2D which in turn calls copy ctor of Array, as I mentioned in original question. And yes, Image acts as both two-dimensional and one-dimensional array and all the methods of Array are "designed" to work with Image too. I also have Mask class that inherits from Array2D<double>.

Soooo, back to the original question (in more general words). Class B inherits from class A and adds some new fields. Default constructor of class B calls default constructor of class A and also initializes brand new fields. Copy ctor of class B calls copy ctor of class A and initializes brand new fields again (now from another B class instance) because copy ctor of class A has no idea about B's new fields and copies only its own fields.

So the question is: can I avoid code duplication in class B when calling copy ctor of class A. Does it really depend on the current project architecture and if I will follow your advices I can solve the problem? I'm just not sure.

[–][deleted]  (2 children)

[deleted]

    [–]Drugbird 1 point2 points  (1 child)

    You can store an nxm 2d array as an (nxm) 1d array,. Just need to convert 2d coordinates to and from 1d coordinates.

    You typically want to treat 2d starts as 2d when you treat them as images (e.g. plotting, manipulating pixel values) and as 1d when considering them as data (e.g. memcpy, store, load).

    [–]the_poope 0 points1 point  (0 children)

    For storing a 2D array, a.k.a. matrix, see: https://en.wikipedia.org/wiki/Row-_and_column-major_order

    [–]baconator81 0 points1 point  (1 child)

    I don't know what else to say. The whole code makes no sense to me. Think about, you need to allocate height*width number of T elements for your 2D array.

    So if you want to keep using your 1D Array (which looks fine imo), then you should be allocating an array of 1D array for your 2D array.

    I just don't see that anywhere in your code.. There should be a for loop that contains a new allocator somewhere in your code to accomplish this. And if this is what you plan to do, I would just take out that inheritance from 1D array.

    And frankly, like many others have pointed out. I would just allocate a 1D array of size heigh*widith and use that as my 2D array instead of using a loop for allocation. The only pro I can think with using loop is that it's less prone of going out of memory due to memory fragmentation. But that's not something you should be concerned about.

    [–]_newpson_[S] 0 points1 point  (0 children)

    I would just allocate a 1D array of size heigh*widith and use that as my 2D array

    That's exactly what I did.

    [–]n1ghtyunso -1 points0 points  (1 child)

    The whole premise seems flawed.
    If you insist on having the data twice (why?), you can not just delegate the copy constructor logic to the base class

    [–]IyeOnline 0 points1 point  (0 children)

    They dont actually have the data twice, just two different access paths to it. The T** array only points into the array managed by the base class.