views:

86

answers:

3

Related to: http://stackoverflow.com/questions/2625719/c-private-pointer-leaking

According to Effective C++ (Item 28), "avoid returning handles (references, pointers, or iterators) to object internals. It increases encapsulation, helps const member functions act const, and minimizes the creation of dangling handles."

Returning objects by value is the only way I can think of to avoid returning handles. This to me suggests I should return private object internals by value as much as possible.

However, to return object by value, this requires the copy constructor which goes against the Google C++ Style Guide of "DISALLOW_COPY_AND_ASSIGN" operators.

As a C++ newbie, unless I am missing something, I find these two suggestions to conflict each other.

So my questions are: is there no silver bullet which allows efficient reference returns to object internals that aren't susceptible to dangling pointers? Is the const reference return as good as it gets? In addition, should I not be using pointers for private object fields that often? What is a general rule of thumb for choosing when to store private instance fields of objects as by value or by pointer?

(Edit) For clarification, Meyers' example dangling pointer code:

class Rectangle {
public:
  const Point& upperLeft() const { return pData->ulhc; }
  const Point& lowerRight() const { return pData->lrhc; }
  ...
};

class GUIObject { ... };
const Rectangle boundingBox(const GUIObject& obj); 

If the client creates a function with code such as:

GUIObject *pgo; // point to some GUIObject
const Point *pUpperLeft = &(boundingBox(*pgo).upperLeft());

"The call to boundingBox will return a new, temporary Rectangle object [(called temp from here.)] upperLeft will then be called on temp, and that call will return a reference to an internal part of temp, in particular, to one of the Points making it up...at the end of the statement, boundingBox's return value temp will be destroyed, and that will indirectly lead to the destruction of temp's Points. That, in turn, will leave pUpperLeft pointing to an object that no longer exists." Meyers, Effective C++ (Item 28)

I think he is suggesting to return Point by value instead to avoid this:

const Point upperLeft() const { return pData->ulhc; }
A: 

It really depends on the situation. If you plan to see changes in the calling method you want to pass by reference. Remember that passing by value is a pretty heavy operation. It requires a call to the copy constructor which in essence has to allocate and store enough memory to fit size of your object.

One thing you can do is fake pass by value. What that means is pass the actual parameter by value to a method that accepts const your object. This of course means the caller does not care to see changes to your object.

Try to limit pass by value if you can unless you have to.

JonH
+4  A: 

The Google C++ style guide is, shall we say, somewhat "special" and has led to much discussion on various C++ newsgroups. Let's leave it at that.

Under normal circumstances I would suggest that following the guidelines in Effective C++ is generally considered to be a good thing; in your specific case, returning an object instead of any sort of reference to an internal object is usually the right thing to do. Most compilers are pretty good at handling large return values (Google for Return Value Optimization, pretty much every compiler does it).

If measurements with a profiler suggest that returning a value is becoming a bottleneck, then I would look at alternative methods.

Timo Geusch
Bah! According to Wikipedia, it appears More Effective C++ talks about this in detail (which I do not have yet.) It's brutal how there is much to learn about C++ quirks before banging out production-ready code. Nevertheless, thank you :)
Glitz
It's worth noting that compilers using RVO generally use it to optimize unnecessary copying of temporaries. They wouldn't necessarily do such a good job at optimizing out the copying for a simple accessor returning a UDT by value.
+2  A: 

First, let's look at this statement in context:

According to Effective C++ (Item 28), "avoid returning handles (references, pointers, or iterators) to object internals. It increases encapsulation, helps const member functions act const, and minimizes the creation of dangling handles."

This is basically talking about a class's ability to maintain invariants (properties that remain unchanged, roughly speaking).

Let's say you have a button widget wrapper, Button, which stores an OS-specific window handle to the button. If the client using the class had access to the internal handle, they could tamper with it using OS-specific calls like destroying the button, making it invisible, etc. Basically by returning this handle, your Button class sacrifices any control it originally had over the button handle.

You want to avoid these situations in such a Button class by providing everything you can do with the button as methods in this Button class. Then you don't need to ever return a handle to the OS-specific button handle.

Unfortunately, this doesn't always work in practice. Sometimes you have to return the handle or pointer or some other internal by reference for various reasons. Let's take *boost::scoped_ptr*, for instance. It is a smart pointer designed to manage memory through the internal pointer it stores. It has a get() method which returns this internal pointer. Unfortunately, that allows clients to do things like:

delete my_scoped_ptr.get(); // wrong

Nevertheless, this compromise was required because there are many cases where we are working with C/C++ APIs that require regular pointers to be passed in. Compromises are often necessary to satisfy libraries which don't accept your particular class but does accept one of its internals.

In your case, try to think if your class can avoid returning internals this way by instead providing functions to do everything one would want to do with the internal through your public interface. If not, then you've done all you can do; you'll have to return a pointer/reference to it but it would be a good habit to document it as a special case. You should also consider using friends if you know which places need to gain access to the class's internals in advance; this way you can keep such accessor methods private and inaccessible to everyone else.

Returning objects by value is the only way I can think of to avoid returning handles. This to me suggests I should return private object internals by value as much as possible.

No, if you can return a copy, then you can equally return by const reference. The clients cannot (under normal circumstances) tamper with such internals.

Thanks! especially on using friends wisely. I understand that const reference doesn't allow private internals to be tampered, however this concerned me: "It doesn't matter whether the handle is a pointer, a reference, or an iterator. It doesn't matter whether it's qualified with const. It doesn't matter whether the member function returning the handle is itself const. All that matters is that a handle is being returned, because once that's being done, you run the risk that the handle will outlive the object it refers to." Is this danger inevitable? Should I use a shared_ptr in such cases?
Glitz
@Glitz it depends on what is meant by handle. If the handle is a pointer or something that allows mutable access, then returning it in any fashion would allow the client to tamper with internals. However, I'm quite sure Meyers is using the term, 'handle', for this particular mutable case. For instance, returning int* const does not prevent the user from tampering with the integer pointee. Could you give a bit more information about your particular case? Some sample code perhaps?
If it is a *handle* in this sense, returning it by value is equally problematic, since a copy of a handle is still basically pointing to the same thing. The only way to avoid this case is to avoid returning the handle at all in any shape or form, and make your class fully responsible for doing anything with it. If you can't avoid that, then you have to make a compromise (but still consider friends and if that doesn't work, at least document the function so that clients aren't tempted to just use the handle and ignore your class).
I added Meyers' example code to the question for clarification
Glitz