Allocating and returning an array of structs from a function

Projects Programming
1

I've been trying to figure out the correct approach for a seemingly simple operation - returning a dynamically allocated array of structs from a function call.

typedef struct Person {
  char const* name;
  int32_t age;
}

Person* findPersons(int& length) {
  length = getNumPersons();
  Person persons[length];
  for(int i = 0; i < length; i++) {
     // do some things
  }
  return persons;
}

calling the method:

Person* persons = findPersons(length);

if I were to call this method I would receive the length and a pointer to the allocated array of structs, however when I access the array I'm getting a core dump (LoadProhibited). I'm assuming this is because the allocated memory is pushed off the stack once the method is done and I can no longer access it.

The standard approach would be to have the caller allocate the array and pass it to the function to be populated, but what happens when I don't know the size ahead of time? Can I malloc() the array and have the caller responsible for releasing the memory?

2

That's not the code you posted is doing. It's returning a pointer to a local variable which is a recipe for disaster as it the array no longer exists once the function returns.

You can, but doing so risks a memory leak. If you're not coding for an AVR processor, you'd be better off returning a std::vector.

3

The 'new' does the same as malloc(), returning a pointer to it in the heap is okay. As long as it is deleted once it is no longer needed.
You may also return a copy of the complete object.
But returning a pointer to a object that is no longer valid after the functions finishes is not possible.

The struct does not contain the characters of the name. It has a pointer to the name. I assume that the name is somewhere else ?

4

That's not the code you posted is doing. It's returning a pointer to a local variable which is a recipe for disaster as it the array no longer exists once the function returns.

Correct, I think I explained the problem with why that wouldn't work. I tried the following approach instead, which didn't really work either and is probably a bad idea as well (malloc):

Person* findPersons(int& length) {
  length = getNumPersons();
  Person* persons = (Person*)malloc(length * sizeof(*persons));
  for(int i = 0; i < length; i++) {
     // do some things
  }
  return persons;
}

int length;
Person* persons = findPersons(length);
for(int i =0; i < length; i++)
  Serial.print(persons[i].name);
free(networks);

not really sure though why the above doesn't work aside from being a bad idea. Some values print, but others are mangled.

5

Please show a complete sketch that we can try and tell which Arduino board you use.

6

The struct does not contain the characters of the name. It has a pointer to the name. I assume that the name is somewhere else ?

Probably a bad example, but the names are copied from the source as String's where I'm getting them. I assumed using PTR's in the structs was ok to do, as most of the examples I found were doing it this way rather than defining a char[]. Maybe I assume wrong here.

7

Malloc has no place in C++: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r10-avoid-malloc-and-free

New would not be a good idea either: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r3-a-raw-pointer-a-t-is-non-owning

however when I access the array I'm getting a core dump (LoadProhibited)

This indicates that you're not using an AVR-based Arduino. Therefore, you should return a std::vector<Person>.

8

This has triple layers now. The String object does not contain the text. It has a pointer to the text in the heap.

9

This indicates that you're not using an AVR-based Arduino

Correct, it's an ESP32 WROOM-32.

10

Then there's no reason whatsoever to return allocated memory as a raw pointer. Use a container like std::vector.

Alternatively, avoid the dynamic allocation altogether and invert the control flow to avoid lifetime issues: https://godbolt.org/z/GxMvG41Eh

template <class Callable>
void foreach_person(Callable callback) {
    static constexpr Person persons[] {
        {"John", 42},
        {"Jill", 43},
    };
    for (const Person &person : persons) {
        callback(person);
    }
}

#include <print>

int main() {
    int32_t sum_of_ages = 0;
    foreach_person([&](Person p) {
        std::println("{} is {} years old", p.name, p.age);
        sum_of_ages += p.age;
    });
    std::println("Their combined age is {}", sum_of_ages);
}
11

yep I think I'll go this route. Pretty rusty with both C and CPP and relearning a lot of things. Thanks!

12

This does not solve the dangling name pointer issue, though. You cannot point it to a local String variable and then return the Person (or vector of Persons).

13

This does not solve the dangling name pointer issue, though. You cannot point it to a local String variable and then return the Person (or vector of Person s).

Yep I understand that thanks :slight_smile: I have it working now after switching to std::vector in my non-example project.

14

@PieterP, I'm afraid that the code does not run on a ESP32. The ESP32 has a "ESP-IDF" for normal C++ (without Arduino), but it is not the same as C++ for a computer.

15

Indeed, it is merely an example to run on Compiler Explorer. On an Arduino, you would use setup() and loop() instead of main(), and Serial.println() instead of std::println().

16

In Arduino mode (not the ESP-IDF mode), it would become this:

struct Person {
    char const* name;
    int age;
};

template <class Callable>
void foreach_person(Callable callback) {
    static constexpr Person persons[] {
        {"John", 42},
        {"Jill", 43},
    };
    for (const Person &person : persons) {
        callback(person);
    }
}

void setup() {
    Serial.begin(115200);
    Serial.println();

    int sum_of_ages = 0;   // "int" to match %d later on
    foreach_person([&](Person p) {
        Serial.printf("%s is %d years old\r\n", p.name, p.age);
        sum_of_ages += p.age;
    });
    Serial.printf("Their combined age is %d\r\n", sum_of_ages);
}

void loop() {
  delay(10);
}

Try it in Wokwi simulation: https://wokwi.com/projects/398177033060581377

17

If I understand the paradigm correctly, instead of passing data out of the function to be operated on, the functionality for that operation is passed into the function that holds the (static) data.

The trouble I'm wondering about is what happens if you need to do this multiple times with different operation functionality? Then, the lambdas will have different types so the template will cause multiple instantiations of the foreach_person() function. Besides the duplication, if the data held within foreach_person() is not constant, then it will get out of sync between the multiple instantiations as it evolves.

@PieterP, is this a valid concern? If so, then perhaps better to not make foreach_person() a templated function but instead accept std::function<void ()> as it's parameter type?

18

Not necessarily static data. That was just an example, in practice, you'd probably generate it on-demand, parse it from a network packet, read it from a file, etc.

Indeed. But unless you're using this function dozens of times and foreach_person() is particularly large, this is unlikely to be an issue. If you nicely encapsulate the logic for generating a single person in a separate function, foreach_person() itself should be a very small function, just a for loop with two function calls in its body. In such a case, the compiler would probably try to inline it and create multiple copies anyway, even if it's not a template.

Sure, but having mutable static data is always a terrible idea.

There may be scenarios where this may make sense, but std::function is heavy machinery, it involves dynamic allocations, and the indirections limit optimization opportunities for the compiler.

You could always use a simpler type erasure approach without dynamic allocations:

https://godbolt.org/z/Gjsz8brn8

Here, the main implementation is an ordinary function (the specific lambdas still ends up in the binary, of course, but foreach_person is only emitted once). At -O2 and -O3, you'll see that the compiler even fully inlines everything into main.

That said, you would need a very compelling reason to add these kinds of micro-optimizations.

In contrast, the std::function version generates way more code, and even at -O3 it is not able to inline the actual function calls into main:

https://godbolt.org/z/s4sYxEbMc

Depending on the size and performance of foreach_person, this may be an acceptable trade-off between maintainability and code size/performance, though.

19

Interesting. In the templated function, when the lambda type_erased is created:

template <class Callable>
void foreach_person(Callable callback) {
    // Necessary if Callable is a function pointer, could be optimized using SFINAE/concepts/if constexpr
    auto wrapped = [&callback](const Person &person) { callback(person); };
    auto type_erased = [](void *context, const Person &person) {
        (*reinterpret_cast<decltype(wrapped) *>(context))(person);
    };
    detail::foreach_person(&wrapped, type_erased);
}

How does decltype(wrapped) work if local variable wrapped isn't captured by the type_erased lambda?

20

The variable is not captured, so you cannot access its value inside of the lambda, but the name is available, so you can still ask for properties that are known at compile time, such as decltype or sizeof.