Do you return const references?

Started by
33 comments, last by okonomiyaki 17 years, 6 months ago
Quote:Original post by NotAYakk
Returning a const& is "the same as" returning a const pointer to a const object.

Do you understand the dangers of returning a const pointer from a class? A non-reference counted const pointer?

Even if the object pointed to is const, if the object pointed to is replaced, reading the const pointer to a const object will generate undefined and unpredictable behaviour.

The fact of the matter is, developer time can be turned into performance boosts. Bugs are turned into developer time. So by writing code that preemptively prevents bugs, you free up developer time, and thus free up developer time to boost performance.

Code to defend yourself against bugs and for flexibility (so you can fix design mistakes) and for speed of coding and for maintainability. Micro-optimizations that also open you up to memory corruption bugs and reduce abstraction don't seem all that tempting to me.


Returning a const pointer is no more dangerous than returning a const reference.

class foo{public:const int& reference()const{    return data;}const int* pointer()const{    return &data;};private:int data;};foo f;assert( foo.reference() == *foo.pointer() );assert( &foo.reference() == foo.pointer() );


Advertisement
Quote:Original post by Nitage

Returning a const pointer is no more dangerous than returning a const reference.

*** Source Snippet Removed ***


Exactly, he's saying they are equally dangerous.
Quote:Original post by okonomiyaki
Quote:Original post by Nitage

Returning a const pointer is no more dangerous than returning a const reference.

*** Source Snippet Removed ***


Exactly, he's saying they are equally dangerous.


Ah, ok.
Quote:Original post by okonomiyaki
Anyway, I've decided to leave it up to the compiler except for certain circumstances where a ton of copying would occur. It annoys me when people tell others to not care about small coding optimizations, and to sloppily write an app and optimize the bottlenecks. Sure, this isn't going save a ton of time, but minor tweaks like this could make a small difference in the end which is good enough for me to care about. Why not make this small difference while I'm coding? I certainly would never go back and optimize an app this way, but while I'm coding I might as well do it as efficiently as possible. Of course, without doing anything stupid that would make it break easily, which in my opinion this is not one of those cases.


Dude, it annoys you when people who have shipped high performance AAA titles give you real-world advice about what to worry about? Really?

Nobody here has suggested that you write anything in a sloppy fashion, but silly little pseudo-optimizations like this are, as others here have mentioned, dangerous, dumb, and ultimately counterproductive. Focusing on getting your algorithms right is going to win you 100x more speedups than whether or not you copy around a few floats.
It's ultimately up to you what you want to do. Returning const references is very commonly used in the game industry. It's not hard to fabricate a potential problem in doing so, but I have never seen such a problem actually happen. Even the worst coders I've known know better than to keep references around to such things as transforms, positions.

You'd actually have to go out of your way to screw up like that anyhow. You have to initialize const &, making them not often useful as class members, unless you plan to initialize it in the constructor initializer list to some placeholder static/global, and I sure as hell wouldn't make a global const ref. In my normal use, and nearly every game engine I've seen, they are strictly temporary access to things such as this, as well as commonly used function parameter modifiers.

There's little reason not to write code from the start like this. You can write bad code to demonstrate why you shouldn't, but it's just examples of bad code. I wouldn't force my whole game to return copies of transforms and positions(which are typically accessed alot), for the almost negligable chance that someone will hold a reference around longer than they should, or get a reference, modify the object, then use the reference(and whose to say they wouldnt want the const ref to reflect the changes anyhow?).

There's a fine line in premature optimization and just smart coding. Some might consider these things as premature optimization, whereas I for example consider them smart coding. I tend to write most accessor functions that are used to access a member variable of a class using the const ref method(non pod type members). Deliberately choosing a faster method up front is not premature optimziation in my book, regardless of whether it would have ended up being a hot spot in your profiling at the end. Using that logic one could argue against using efficient containers until one profiles and determines that the containers are indeed slow in a profile. That doesn't make any sense. Returning by const & isn't a micro optimization. It's a well known coding practice that has been recommended by many sources for many years. Just because the yields may not be as great on todays computers doesn't make it any less of a decent practice. Whether you choose to use it or not is up to you, and you should certainly be aware of what it means to do it either way.

Quote:Orignially by JasonBlochowiak
Dude, it annoys you when people who have shipped high performance AAA titles give you real-world advice about what to worry about? Really?


I have no idea about the personal life or programming background of anyone on this forum (well, 99% of people here).

Quote:Orignially by DrEvil
There's a fine line in premature optimization and just smart coding. Some might consider these things as premature optimization, whereas I for example consider them smart coding.


Thank you, that's exactly what I'm saying. You can tell people to focus on the algorithm all they want in languages such as Java, VB, C#, etc., but in a language like C++, there are simply smarter (and faster) ways of doing things, and that I'm researching: when it's appropriate, and when it's not.

Thanks for your post DrEvil, that's exactly how I was thinking! Ultimately, I'm not going to do this though, except for a few methods that will be called thousands of times per frame that will be copying too much.
Quote:
You'd actually have to go out of your way to screw up like that anyhow. You have to initialize const &, making them not often useful as class members, unless you plan to initialize it in the constructor initializer list


No you don't. The member does not have to be a const reference (or a reference at all); you just need to return one, which, as you point out, is done all the time, usually under the guise of "it needs to be fast!" (the perrenial problem of game developers).

Quote:
(and whose to say they wouldnt want the const ref to reflect the changes anyhow?).

If they want changes to be reflected, "const" is inappropriate.

Quote:
Deliberately choosing a faster method up front is not premature optimziation in my book,

Maybe, maybe not. But in this case, it tends to be bad design, since it breaks encapsulation by exposing implementation details. Anything that makes your API easier to use incorrectly should be scrutinized.

Quote:
I wouldn't force my whole game to return copies of transforms and positions(which are typically accessed alot), for the almost negligable chance that someone will hold a reference around longer than they should, or get a reference, modify the object, then use the reference

If you can leverage language features to error check or error proof your API for you, you should.

In this case, the object itself could be made cheap to copy (by employing some kind of copy-only-on-write implementation internally). Thus, you'd end up with the best of both worlds -- a safer interface, and a fast copy.
Quote:
No you don't. The member does not have to be a const reference (or a reference at all); you just need to return one, which, as you point out, is done all the time, usually under the guise of "it needs to be fast!" (the perrenial problem of game developers).


Not quite sure what you mean by this, can you elaborate? If it's not a reference then you cant access bad memory like your example, and I've never seen such things returned by *. If someone were to try to hold on to a reference for longer than the objects duration they would either need to write a bad block of code, as per your example, or keep the reference around, in the form of a member of another class, global, etc. If they keep a copy of the position, it ceases to be a bug. If they are playing with an object post deletion, that is unlikely to be the only bug they have.

Quote:
Maybe, maybe not. But in this case, it tends to be bad design, since it breaks encapsulation by exposing implementation details. Anything that makes your API easier to use incorrectly should be scrutinized.


I really don't like these sort of comments. x is bad design, singletons, accessors/modifiers, globals, goto, etc. How exactly does it break encapsulation? Because you can safely assume the position is a vec3 member if it returns a const & ? What difference does that make?

Quote:
If you can leverage language features to error check or error proof your API for you, you should.


Sure, but fabricating some example that is dangerous isn't very productive either, and the chances that it will come up are pretty low in my experience. Most programmers know better

Quote:
In this case, the object itself could be made cheap to copy (by employing some kind of copy-only-on-write implementation internally). Thus, you'd end up with the best of both worlds -- a safer interface, and a fast copy.


At what overhead cost to the object? Sounds like an overengineered solution to an insignificant problem. I can count the times such an issue has come up in my professional career on 0 hands. Not saying either way is more right or wrong, just disagreeing with your reasonings for why. To me it's 95% preference, and 5% performance. The potential for a problem is probably as insignificant today as the average cpu savings, though I stick with it for things like accessors for positions, orientations, transforms, etc.

On the flip side, Microsoft had been recommending at several conferences to return by value, even up to matrix sized objects, because the register size on the 360 makes it a faster technique. I'm not sure how this applies to the other consoles though, or even how much faster it supposedly is on the 360.
Here is a block of code that would execute undefined behaviour under "return const&", and work fine under "return by value":

int foo::get_value();
or
int const& foo::get_value();

foo* f = new foo();const int& data = f->get_value();delete f;printf("%d\n", data);


Or, for a more practical problem:
void modify_foo( const int& max, const int& val, foo* f ) {  f->set_value(val);  if (f->get_value() > max) { f->set_value(max); }}modify_foo( f->get_value(), 7, f );


Change the signature of get_value(), and modify_foo does seriously different things. If get_value() returns a const reference, the max check no longer does anything. If get_value() returned by reference, the max check works.

In effect, by returning a const reference your code now silently behaves very strangely when interacting with other code that passes const references to read only parameters, rather than making copies of them.

Quote:If they want changes to be reflected, "const" is inappropriate.


"const" simply means I won't change it. It says nothing about someone else not changing it.
Quote:
Not quite sure what you mean by this, can you elaborate? If it's not a reference then you cant access bad memory like your example

Yes, you can. My example does so. Note that the member to which a reference is being created (bar) is not itself a reference. The portion of your post I was replying to ("You have to initialize const &, making them not often useful as class members,") implied to me that you thought the member needed to be a reference -- but it doesn't. You simply have to create a reference to a member, and then cause that member to go away somehow. Then you have a reference that refers to invalid memory.

A reference (except in the case of a reference to a temporary, which this is not) does not "pin" the object it references and keep it alive in any way.

Quote:
If someone were to try to hold on to a reference for longer than the objects duration they would either need to write a bad block of code, as per your example,

The problem is that, while the code that I wrote is "bad," it's not really obviously so. Well, it's not that obvious, at least. And when it occurs in production code, it is even less obvious.

Quote:
I really don't like these sort of comments. x is bad design, singletons, accessors/modifiers, globals, goto, etc.

Note that I didn't make any such blanket statement. I said (and specifically emphasized) that it tends to do so, which is much less of a generalization than "it is bad design;" it admits the possibility that there is a valid use for it.

Quote:How exactly does it break encapsulation? Because you can safely assume the position is a vec3 member if it returns a const & ? What difference does that make?

To some extent, yes, you can make that assumption and that is a minor encapsulation violation. That isn't a huge deal, though, because after all you could just look at the header; that's only a literal break of encapsulation. The bigger issue regarding encapsulation breakage is that it means that fundamental changes to the internals of the member in question might cause breaking changes to the public interface. Remember, the public interface of a class is the hardest thing to change, so its very important to get it right (or as close as you can to right) the first time.

I mentioned this briefly already, but I'll expand on it. Consider a situation in which we return a reference to a internal field. Though it appears benign, in permitting this in our interface we have restricted our set of legal implementations to those that cache the value of that field somehow. If we ever need to change the implementation of the class to calculate that field's value on demand (perhaps because we have very tight memory restrictions, an equally valid possibility in any context where "return-by-reference-rather-than-value" provides signifigant performance gains), we can't. That value must be stored or cached somewhere (otherwise we'd have to return a reference to the temporary calculated value, obviously a problem), or we have to break the interface.

Admittedly, this is somewhat less of a concern for hobbyist or amateur developers who might not have the burden of massive amounts of legacy code that would be adversly affected by a breaking interface change.

It also alleviated somewhat by returning a const reference, since changing the interface to return by value will silently work properly in a good deal of cases -- but not neccessarily all.

And I don't believe being a hobbyist is an excuse for making poor design choices.

Quote:
Sure, but fabricating some example that is dangerous isn't very productive either, and the chances that it will come up are pretty low in my experience. Most programmers know better

Holding the reference happens frequently. My example is trivial in the interest of illustration; in practice the site that causes the reference to become invalidated (the "delete") is usually far from the site at which the reference was acquired, buried behind a few levels of function calls. It doesn't happen because the programmer is stupid or doesn't know better, it happens by accident because the API was easy to use incorrectly.

Quote:
At what overhead cost to the object? Sounds like an overengineered solution to an insignificant problem.

Copy-on-write isn't that hard to implement, and can pay dividends in areas other than returning by value from accessors. It was done, and rather quickly, for a number of objects on projects I've worked on. But yes, in general it is an insignifigant problem -- so it's often better to do it the safer way.

Quote:
"const" simply means I won't change it. It says nothing about someone else not changing it.

I know; that's what I was getting at. I read "they" in DrEvil's post as the implementor of the class's interface, so "they" would make the return const if "they" didn't want "I" (me) to change it. If "they" want me to change it (through that returned reference), const is not appropriate.

This topic is closed to new replies.

Advertisement