0

My apologize for asking a basic question but I really could not find a suitable answer for this. I have inherited a C++ code where a part of it has a 2D allocation function Allocate2D in the form of:

float** Allocate2D(long long sizeX, long long sizeY)
{
    float** p = new float*[sizeY];
    p[0] = new float[sizeX*sizeY];

    for (long long z = 0; z < sizeY; z++) {
        p[z] = p[0] + z * sizeX;
    }
    return ptr;
}

int main(){
    long long nX = ... ; 
    long long nY = ... ; 
    short** A = (short **)Allocate2D(nX, nY);
    // do stuff with A ...
}

I have two questions on this:

1- Is Allocate2D creating a 2D array with size sizeX x sizeY?

2- Is this common/legal/good practice to allocate a 2D short array such as A when Allocate2d is defined for float**?

If_You_Say_So
  • 1,195
  • 1
  • 10
  • 25
  • It's worth understanding the technical point that there is no such thing as a 2D array. You have an array of pointers to floats (hence the `**`), and each pointer is pointing to their own array of floats. It's an 'array of arrays.' I bring this up because it's a one-time job to put this in a class, with memory allocated contiguously. – sweenish Mar 25 '21 at 18:33
  • Well that code is just awful – Hatted Rooster Mar 25 '21 at 18:34
  • @sweenish> It is not an array of array. An array of pointers is just that: an array of pointers. `int[42][42]` would be an array of arrays. – spectras Mar 25 '21 at 18:38
  • If we're being pedantic, sure. The line `p[0] = new float[sizeX*sizeY];` takes that pointer and creates an array. So what if I said **exactly** what you just did, but then describe the end-product? Where's the complaint? The function is utterly broken anyway. It's hardly worth arguing over. – sweenish Mar 25 '21 at 18:42
  • It's not being pedantic. For instance, you can use a `float **` to index into an array of pointers. You **cannot** use a `float **` to index into an array of arrays. Those are different things, with a different layout in memory and different rules. – spectras Mar 25 '21 at 18:43
  • Yuck. See if it's politically possible to replace it with [something like what this linked answer does](https://stackoverflow.com/a/2076668/4581301). – user4581301 Mar 25 '21 at 18:49
  • @user4581301 - Thanks for the suggestion. I can and I will but I would like to understand what is happening here before changing it. – If_You_Say_So Mar 25 '21 at 18:52
  • *Is this common/legal/good practice* I can only speak for myself, but this would fail code review with the recommendation that `Allocate2D` at the very least become a templated function. – user4581301 Mar 25 '21 at 18:52
  • @spectras Well, thank you for pointing that out. Tested it out, and while I can do it with `T*` just fine, my assumption (always the cause of my errors) was that `T**` would be no different. – sweenish Mar 25 '21 at 18:57
  • 1
    What is happening. One array of pointers is being allocated and one big array of `float`s (too much data in the case of the common size of `short` being 16 bit). The array of `float`s is cut up into rows with each row assigned to a pointer in the array of pointers, which can then be used like a 2D array. This keeps the data nice and contiguous and often gives a significant performance advantage over `new`ing each row separately (search term: spatial locality). – user4581301 Mar 25 '21 at 18:57
  • @sweenish> I think we C++ devs all got tripped by this along the way at least once. It comes from the unfortunate interaction between of how C++ arrays work, and the automatic array-to-pointer conversion, aka decay (which only makes sense on outtermost level). Together they make the whole matter very confusing until you've spent some time deciphering the mess. Happily that's a one-time thing, once one figures it out it's for good. – spectras Mar 25 '21 at 19:04
  • Not me. I got tripped over this back when I was a C dev. – user4581301 Mar 25 '21 at 19:10
  • So I read about spatial locality as @user4581301 suggested and now I am not too sure why everyone says this is a poorly implemented allocation function. I do see the point in templating the function though. – If_You_Say_So Mar 25 '21 at 19:41
  • 1
    It's better than the worst case of memory scattered all over the place, but this solution has a redundant array of pointers and the the pointer-chasing it brings with it. It also requires manually managing both of the allocations, and that can be a lot trickier than it seems. The wrapper class I linked to above has only one array, that array is managed by `std::vector` so the memory is always put away when you're done with it (unless you go out of your way not to), and the pointer chasing is replaced by some really simple math. – user4581301 Mar 25 '21 at 20:53
  • 1
    The downside is it doesn't look exactly like an array, you have to index it `matrix(x,y)` rather than the more familiar `matrix[x][y]`. – user4581301 Mar 25 '21 at 20:53
  • @user4581301 - I see. Thanks very much! – If_You_Say_So Mar 25 '21 at 21:04

1 Answers1

2

  1. Yes. It does depend on what exactly you mean by "2D array". There is more than one way to create a data structure for storing data that is indexed by two dimensions. One can dereference A with two dimensions, as in A[y][x], and it will work as expected. Also, there will be a pointer in A[0] that points to a contiguous array of sizeX x sizeY floats, with the X dimension varying fastest. This is the memory layout one would also get with float B[sizeY][sizeX]. However, the types of A and B, (ignoring float vs short), are not the same. A is a pointer to an array of pointers, while B decays to a pointer to a float.

  2. No. It's not legal to cast a pointer to point to a different type. It breaks pointer aliasing rules. Also, since sizeof(float) is almost certainly not equal to sizeof(short), the arrays will be the wrong size, i.e. it will be nX*2 × nY. However, since a float* is never dereferenced into a float in what we can clearly see is the very limited lifetime of the float pointers, there will realistically not be a problem.

So this will work, other than the array being twice as large as it should be. But turning the float pointers into short pointers is not strictly legal and it's ugly. It would be much better, and quite easy, to write a function template that gets the type correct.

TrentP
  • 4,240
  • 24
  • 35
  • Thank you. Actually I realized that the author of the code in fact wanted to allocate an array of size `2*nx*ny` in this case and that is why they cast to `short`. – If_You_Say_So Mar 26 '21 at 20:06
  • If this was a function template, then not only would it avoid the pointer aliasing undefined behavior, it would also get the dimensions correct. It's a great example of when to use a template. – TrentP Mar 27 '21 at 21:41