Is this the correct way to return an array of structs from a function?

Question

As the title says (and suggests), I'm new to C and I'm trying to return an arbitrary sized array of structs from a function. I chose to use malloc, as someone on the internet, whose cleverer than me, pointed out that unless I allocate to the heap, the array will be destroyed when points_on_circle finishes executing, and a useless pointer will be returned.

The code I'm presenting used to work, but now I'm calling the function more and more in my code, I'm getting a runtime error ./main: free(): invalid next size (normal): 0x0a00e380. I'm guessing this is down to my hacked-together implementation of arrays/pointers.

I'm not calling free as of yet, as many of the arrays I'm building will need to persist throughout the life of the program (I will be adding free() calls to the remainder!).

xy* points_on_circle(int amount, float radius)
{
  xy* array = malloc(sizeof(xy) * amount);

  float space = (PI * 2) / amount;

  while (amount-- >= 0) {
    float theta = space * amount;

    array[amount].x = sin(theta) * radius;
    array[amount].y = cos(theta) * radius;
  }

  return array;
}

My ground-breaking xy struct is defined as follows:

typedef struct { float x; float y; } xy;

And an example of how I'm calling the function is as follows:

xy * outer_points = points_on_circle(360, 5.0);
for(;i<360;i++) {
    //outer_points[i].x
    //outer_points[i].y
}

A pointer in the right direction would be appreciated.

Answer 1

Allocating memory in one function and freeing it in another is fraught with peril.

I would allocate the memory and pass it (the memory buffer) to the function with a parameter indicating how many structures are allowed to be written to the buffer.

I've seen APIs where there are two functions, one to get the memory required and then another to actually get the data after the memory has been allocated.

[Edit] Found an example: http://msdn.microsoft.com/en-us/library/ms647005%28VS.85%29.aspx

Answer 2

I would say that this program design is fundamentally flawed. First of all, logically a function which is doing calculations has nothing to do with memory allocation, those are two different things. Second, unless the function that allocates memory and the one that frees it belong to the same program module, the design is bad and you will likely get memory leaks. Instead, leave allocation to the caller.

The code also contains various dangerous practice. Avoid using -- and ++ operators as part of complex expressions, it is a very common cause for bugs. Your code looks as if it has a fatal bug and is writing out of bounds on the array, just because you are mixing -- with other operators. There is never any reason to do so in the C language, so don't do it.

Another dangerous practice is the reliance on C's implicit type conversions from ints to float (balancing, aka "the usual arithmetic conversions"). What is "PI" in this code? Is it an int, float or double? The outcome of the code will vary depending on this.

Here is what I propose instead (not tested):

void get_points_on_circle (xy* buffer, size_t items, float radius)
{
  float space = (PI * 2.0f) / items;
  float theta;
  signed int i;

  for(i=items-1; i>=0; i--)
  {
    theta = space * i;

    buffer[i].x = sin(theta) * radius;
    buffer[i].y = cos(theta) * radius;
  }
}

Answer 3

EDIT: You are returning the array correctly, but ...

---

Consider you're making an array with 1 element

xy *outer_points = points_on_circle(1, 5.0);

What happens inside the function?
Let's check ...

  xy* array = malloc(sizeof(xy) * amount);

allocate space for 1 element. OK!

  while (amount-- >= 0) {

1 is greater or equal to 0 so the loop executes (and amount gets decreased)
after setting array[0] you return to the top of the loop

  while (amount-- >= 0) {

0 is greater or equal to 0 so the loop executes (and amount gets decreased)
You're now trying to set array[-1], which is invalid because the index refers to an area outside of the array.