Address of local variable returned--can ignore warning without detriment but what's the proper way?

Question

I have a class Particle:

class Particle {
private:
    float x, y, z;
    // ...
public:
    // ...
    float* getPos() {
        float p[3] = {x, y, z};
        return p;
    }
    // ...
};

I would call this method like:

Particle a = Particle();
// ...
float* pos = a.getPos();

And then reference the position elements with pos[0] through pos[2].

g++ spouts warning message as stated in the title. But the functionality is exactly how I want it: returning an array. Why does the warning exist and is there a "proper" way to do it?

Possible duplicate of [Can a local variable's memory be accessed outside its scope?](https://stackoverflow.com/questions/6441218/can-a-local-variables-memory-be-accessed-outside-its-scope) — πάντα ῥεῖ, Apr 07 '19 at 19:58
If local variable isn't static - you'll get undefined behavior, and quite likely a crash in runtime. Either return instance created by new operator(and don't forget to free the memory via delete), also you can use std::unique_ptr for it... Or just return by value — Dmitry Gordon, Apr 07 '19 at 20:01
Return a copy of the array instead. Consider using `std::array`. — Cruz Jean, Apr 07 '19 at 20:03
You are returning a pointer to a chunk of memory. That chunk of memory has to be allocated, data has to be copied into it, and then at some point after the pointer is finished being used, the block of memory has to be freed. How are you expecting the block of memory to be freed and how are you expecting it to be determined when it can be freed? — David Schwartz, Apr 07 '19 at 20:42

score 2 · Answer 1 · answered Apr 07 '19 at 20:16

2

You can't return a C-array like that, return std::array instead:

std::array<float, 3> getPos() {
    std::array<float, 3> p = {x, y, z};
    return p;
}

You'll need to include <array> for that.

answered Apr 07 '19 at 20:16

Hatted Rooster

35,759
6
62
122

score 1 · Answer 2 · answered Apr 07 '19 at 20:03

You're not returning an array. It's impossible to return an array in C++. You're returning a pointer to an array which no longer exists. Hence the warning.

You could make the array a part of your class and return a pointer to that. In general I wouldn't call that good design

class Particle {
private:
    float pos[3];
    // ...
public:
    // ...
    float* getPos() {
        return pos;
    }
    // ...
};

You could return a vector<float> instead. You could return an array<float,3> instead. You could ask yourself why you need this.

score 1 · Accepted Answer · answered Apr 11 '19 at 22:33

Personally, I'd skip std::array/std::vector here, because in your particular case, the position of each value imposes independent meaning. In general, sequence types have ordering, tuples have structure; if the element count is fixed (and often heterogeneous) and sorting (or otherwise reordering the values) is intrinsically nonsensical (e.g. in the case of a coordinate, swapping the x and y values changes the meaning), then a tuple makes more sense.

In this case, you could just declare:

std::tuple<float, float, float> getPos() {
    // C++17 or higher allows list initialization
    return {x, y, z};

    // Pre-C++17 you use the std::make_tuple helper
    return std::make_tuple(x, y, z);
}

The advantage here is that you can then unpack the result in the caller easily, either with std::tie:

float x, y, z;

std::tie(x, y, z) = a.getPos();

or on C++17 or higher with structured bindings, it's even nicer, since you can declare and initialize the variables with auto, rather than declaring with explicit types, then reassigning with tie:

auto [x, y, z] = a.getPos();

You can store the tuple itself and use std::get if you prefer, but unpacking to useful names rather than obscure std::get indices usually makes for much cleaner code.

score 0 · Answer 4 · answered Apr 07 '19 at 20:13

0

p[3] will be destroyed when it goes out of scope so you shouldn't return a pointer to it. Either return a std::array<float, 3> by value or consider making a class for positions too, and return a Position object, or a reference to it. Example:

struct Position {
    float x, y, z;
};

class Particle {
private:
    Position m_pos;
    // ...
public:
    // ...
    Position const& getPos() const { return m_pos; }
    // ...
};

answered Apr 07 '19 at 20:13

Ted Lyngmo

93,841
5
60
108

If I were pedantic I'd say that `p[3]` never existed in the first place ;-). – Peter - Reinstate Monica Apr 07 '19 at 20:18

Jonathan Mee · Answer 5 · 2019-04-07T20:57:34.550

I'd suggest that you're function is indicative of poor design. Provide getter methods an allow the user of your class to access member variables:

class Particle {
private:
    float x, y, z;
public:
    float GetX() const { return x; }
    float GetY() const { return y; }
    float GetZ() const { return z; }
};

Given const Particle a this will let you initialize an array as follows: const float pos[] = { a.GetX(), a.GetY(), a.GetZ() }

Creating a Particle method to allow the user to populate a float[] will encourage the user toward one of the following bad practices:

float* Particle::GetPos() const { return new[3]{ x, y, z }; } creates dynamic memory without clearly informing the caller that the memory needs to be released
array<float, 3U> Particle::GetPos() const { return { x, y, z }; } requires the allocation and creation of a temporary to populate a float[]
void Particle::GetPos(float* param) const { param[0] = x; param[1] = y; param[2] = z; } misses the opportunity for constant arrays and incurs potential caller misuse, as it's not clear that param must have room for at least 3 floats

Address of local variable returned--can ignore warning without detriment but what's the proper way?

5 Answers5