2

Please, help me with such common task as to write function that must return array. I spent 2 hours with solving this problem, but no results. Here is my code:

 VERTEX* ConstructVertices(float R, unsigned int Slices)
{
    //const unsigned int n = static_cast<const unsigned int>(Slices);
    VERTEX *__vertices = new VERTEX[100];

    float dx = (2 * R) / Slices;
    float dz = dx;
    float x, y, z;
    x = -R;
    y = 0.5f;
    z = 0;
    int x_sign = 1;
    int z_sign = -1;

    __vertices[0].Color = D3DXCOLOR(0.0f, 1.0f, 0.0f, 1.0f);
    __vertices[0].Pos = D3DXVECTOR3(x, y, z);

    for (int i=1; i<Slices * 4 + 1; i++)
    {
        __vertices[i].Color = D3DXCOLOR(0.0f, 1.0f, 0.0f, 1.0f);

        y = -y;

        if (y < 0)
        {
            __vertices[i].Pos = D3DXVECTOR3(x, y, z);
            continue;
        }

        x += x_sign * dx;
        z += z_sign * dz;

        x = round_up(x, 2);
        z = round_up(z, 2);

        if (abs(x) == abs(R))
        {
            x_sign = -x_sign;
        }
        if (abs(z) == abs(R))
        {
            z_sign = -z_sign;
        }

        __vertices[i].Pos = D3DXVECTOR3(x, y, z);   
    }

    return __vertices;
}

Code for accessing the array:

VERTEX *vertices = new VERTEX[100];
vertices = ConstructVertices(1, 10);

With Watches window i can see values like vertices[0], vertices[1],.. But i can't see it as an array & the main is sizeof(vertices) returns 4 instead of 160!!

Thank a lot!

oler117
  • 89
  • 1
  • 8
  • 1
    Instead of 100: `unsigned int n_vertices = Slices * 4 + 1;` – Joop Eggen Dec 15 '12 at 18:54
  • 1
    You should change the name of `__vertices`. http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier – chris Dec 15 '12 at 19:22

4 Answers4

1

Just return a std::vector<VERTEX>.

Returning a pointer to memory allocated on the heap has several major problems:

  1. The memory can easily leak when not being taken care of properly.
  2. From the interface it isn't clear how the memory is to be released and the compiler can't help using the right approach (although using delete[] a; is a likely guess at what the correct approach is).
  3. You cannot determine the size of the returned array from the pointer.

Clearly, you can create a class which provides suitable operations but this is what std::vector<T> is. If you need a pointer to the elements just use the address of the first element of a non-empty vector (&a[0]) or a.data(). The latter was introduced with C++ 2011 and has tge advantage of also working with empty arrays (where the returned pointer can't be dereferenced, of course).

Coding Mash
  • 3,338
  • 5
  • 24
  • 45
Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380
  • I think an explanation would be more helpful to a beginner. – Ed S. Dec 15 '12 at 18:52
  • I need an array as the end result. – oler117 Dec 15 '12 at 18:56
  • @EdS. It is, never the less, the only correct answer. C style arrays are broken, and should only be used in very limited cases, none of which are likely to be encountered by a beginner. – James Kanze Dec 15 '12 at 19:02
  • @user1906714 What do you mean by "I need an array"? The usual way to declare an array of `T` in C++ is `std::vector`, and if your course requires anything else, then the course is broken. – James Kanze Dec 15 '12 at 19:04
  • 1
    @JamesKanze: But if you don't explain *why* a vector is preferable then you haven't actually taught anything; you are simply encouraging a cargo-cult style of programming. This person is obviously a beginner, and I doubt that the assignment allows for a vector (well, assuming that it *is* an assignment, perhaps not). Regardless of what professional practices are, everyone writing C++ code should *understand* why they are doing what they are doing. – Ed S. Dec 15 '12 at 19:05
  • @EdS. You can't teach everything at once; when you present `int`, you don't explain why you shouldn't use `double` (which the pupil has probably never seen). Similarly, when you present `std::vector`, you don't go into issues concerning dynamic allocation (which the pupil has certainly never seen) or implicit conversion to pointer (since the pupil will not have seen pointers yet either). – James Kanze Dec 15 '12 at 19:08
  • 1
    @JamesKanze: Well, we disagree. When you tell someone to use a vector you *should* explain why. They should already have a basic understanding of manual memory management and it's difficulties. Start at the beginning, you'll create much more competent software developers. Telling a beginner nothing but "use a vector" isn't helpful and I see it happen way too often. Understanding concepts is more important than solving the this simple implementation issue. – Ed S. Dec 15 '12 at 19:09
  • I just forgot about vector because of in every example that I read on current topic (VertexBuffers in DirectX) arrays are used... – oler117 Dec 15 '12 at 19:17
  • 1
    If uou need an "array" (probably a pointer to the first element), you can take the address of the first element of a non-empty array (e.g., `&a[0]`) or, when using C++ 2011, you can use the `data()` member (which does the same but also works with empty vectors). – Dietmar Kühl Dec 15 '12 at 19:23
  • @EdS. The reason you use vector is simple: it is the standard C++ way of declaring an array. And pointers and dynamic allocation aren't presented until way after you've seen vector in any decent C++ course. – James Kanze Dec 17 '12 at 01:26
  • @JamesKanze: You say that is if I'm the one who doesn't understand why one would use a vector. I assure you that I do in fact understand, that was not the point. – Ed S. Dec 17 '12 at 05:58
1

In your code you first allocate memory dynamically:

VERTEX *vertices = new VERTEX[100];
vertices = ConstructVertices(1, 10);

And then in the function your allocate memory with new:

//const unsigned int n = static_cast<const unsigned int>(Slices);
VERTEX *__vertices = new VERTEX[100];

And finally you return this pointer and replace the first pointer you created. The memory get allocated twice.

Étienne
  • 4,773
  • 2
  • 33
  • 58
0

"the main is sizeof(vertices) returns 4 instead of 160!!" That's because vertices is a pointer, and obviously you are using a fancy high tech 32-bit computer, so the size of a pointer is 4 bytes.
Another point is:

VERTEX *vertices = new VERTEX[100];
vertices = ConstructVertices(1, 10);

the first assignment is useless, because overwritten by the second one without beeing used ever, so you just leak memory with 100 VERTEX.

pbhd
  • 4,384
  • 1
  • 20
  • 26
  • Why VERTEX vertices[] = {{D3DXVECTOR3(-1.0f, 0.5f, 0.0f), D3DXCOLOR(1.0f, 0.0f, 0.0f, 1.0f)}}; sizeof(vertices) returns not 4??? – oler117 Dec 15 '12 at 19:02
  • Grmpf. Then it's not a pointer (ok, its also a pointer, but the debugger knows, that it points to an array), then its an array whos size is defined by the initializers you pass. But saying VERTEX *vertex, you just say this thing points so a VERTEX. In your case your pointer points to the first VERTEX in an array, so vertices[1] would be the next VERTEX. – pbhd Dec 15 '12 at 19:07
  • I know this. But I don't know how can I get an array from pointer pointing on its first ellement? – oler117 Dec 15 '12 at 19:10
0

Instead of this

VERTEX *vertices = new VERTEX[100];
vertices = ConstructVertices(1, 10);

add the vertices array as an argument to the function and tell the function the size

VERTEX *vertices = new VERTEX[100];
ConstructVertices(1, 10,vertices,100);

then inside your function fill the passed array

void ConstructVertices(float R, 
                       unsigned int Slices,
                       VERTEX* vertices, 
                       size_t verticesArraySz )
{
    //const unsigned int n = static_cast<const unsigned int>(Slices);

    float dx = (2 * R) / Slices;
    float dz = dx;

...

do not use names like __vertices, it is not a good habit because normally compiler specific things use that syntax.

AndersK
  • 35,813
  • 6
  • 60
  • 86