6

Assume I have the following code snippet for GPoint, which has a copy constructor, assignment operator, and destructor. The same for GCircle and this has a function named GetCentre() which returns a copy of the Gpoint object(Centre).

In main or ButtonClick(), as below is it safe/valid to call GPoint &Centre = circle.GetCentre()? By doing this (if valid) we would save the time of calling assignment operator!.

class GPoint
{
public:

    GPoint();
    virtual ~GPoint();
    GPoint(double p_dX, double p_dY);
    GPoint (const GPoint &Source);
    GPoint& operator = (const GPoint &point);

public:
    double c_y;
    double c_x;


};

class GCircle//:public GShape
{

public:
    GCircle();
    GCircle(GPoint p_point, double p_dRadius);
    ~GCircle(){}


    operator GPoint&();
    operator double&();

    double& GetRadius() const ;
    GPoint  GetCentre() const {return c_Centre;}  //Return copy Not a reference 

public:
    double  c_dRadius;
    GPoint  c_Centre;
};


Dlg::ButtonClick()
{
    GPoint Point1(10,2);
    GCircle circle(Point1, 100);//100 is the radius.

  **GPoint &Centre = circle.GetCentre();**   //is this reference safe/valid or invalid

}
madhead
  • 31,729
  • 16
  • 153
  • 201
Sal
  • 71
  • 2
  • 4
    It's a reference to the copy returned by your accessor. It's valid for the scope of `ButtonClick` but may not be what you intended. – AJG85 Dec 06 '11 at 21:39
  • Forgot to mention that I have traced the calling of ~GPoint() destructor when ButtonClick() gets out of scope and was called THREE times so, is it possible to conclude that is a valid refernce inside ButtonClick() scope? – Sal Dec 06 '11 at 21:40
  • Does it compiler? Doe's not look like it should. As you have a reference to a temporary. So no it is not OK. – Martin York Dec 06 '11 at 21:40
  • 2
    @AJG85: The code is not valid C++ (even if VS accepts it), and to be precise in the wording it binds the reference to an unnamed variable created by the compiler at the place of call. – David Rodríguez - dribeas Dec 06 '11 at 21:43
  • Yes, it does and works as I want it but still no 100% confident. The idea here for me. I don't want to access GCircle memebers I want to return copy. As I said there is copy constructor for GPoint and Assignment operator (=). So, when GetCentre() is called it will make a copy of Centre(GPoint) then return it, to GPoint &Centre = GetCentre() ====>>> GPoint &Centre = (Copy of Centre) . Is Centre valid reference here? I could see its destructor called when ButtonCLick gets out of scope.! – Sal Dec 06 '11 at 21:46
  • Why are you explicitly calling a copy accessor (return by value) if what you want is a reference (which you already provide an operator for)? – Mordachai Dec 06 '11 at 21:48
  • 1
    @Sal: Your code depends on non-standard behavior in Visual Studio, and will be rejected in any other compiler, even if it was valid (i.e. if the reference was `const GPoint& centre = circle.GetCentre();`) the cost is *equivalent*: semantically the life extension of the rvalue is performed by creating an unnamed variable and *copying* into it (semantically, in practice the copy is elided in *both* cases). There is no point at all in doing that, as the cost will be the same and it will be more surprising for maintainers of the codebase in the future. – David Rodríguez - dribeas Dec 06 '11 at 21:51
  • 1
    Note that `GPoint Centre = circle.GetCentre();` would never invoke the assignment operator, it would potentially invoke the _copy constructor_. – ildjarn Dec 06 '11 at 21:51
  • 1
    @David Rodríguez - dribeas: It's certainly not a good idea. With certain compiler optimization enabled this will become undefined behavior. There are many other things wrong with these objects I'm uncertain what the actual problem is that he isn't asking about. – AJG85 Dec 06 '11 at 21:58
  • 1
    @Sal I recommend you take a look at Scott Meyers' Effective C++, Item #22 "Pass and return objects by reference instead of by value". That whole book would be a good read for you (or any C++ developer). – Mordachai Dec 06 '11 at 22:19
  • @AJG85 That's what I would expect. I know that I have not used &Centre in ButtonClick() but this just code snippet. So, it is safe to assume Centre is valid in the scope of ButtonClick()! – Sal Dec 06 '11 at 22:33
  • 1
    @Sal: Don't write code like this Sal. David is correct. It just so happens that in debug builds of MSVC this will compile and appear to work but that doesn't make it any less wrong. My answer below shows a reference snippet which is more syntactically correct but still so very wrong from a design and style point of view. – AJG85 Dec 06 '11 at 22:37
  • @Mordachai, I know what you mean. But I don't want the outer world to change the members of GCircle and don't have the chance to change the function prototype to return const GPoint& for compatibility issue. – Sal Dec 06 '11 at 22:45
  • @Sal then I think you need not worry about the cost of the copy. Either you can use the existing return by reference and just use const & to avoid changing its guts, or use the return by value and not worry about the copy (which is totally trivial) – Mordachai Dec 07 '11 at 00:52

5 Answers5

6

That code is not valid C++ (even if VS accepts it), as you cannot bind a non-const reference to an rvalue (the temporary returned by the function).

As of the particular question of performance, and considering that you were binding a const reference, there is no advantage at all. The compiler will create an unnamed variable in the calling function and then bind the reference to that, so the copy is performed anyway.

To clarify a bit on the copy is performed anyway, the copy will or will not be performed depending on whether the compiler can elide it, and in general it can. All compilers I know, implement the calling convention for your object (too large for registers) by allocating the object in the caller stack and passing a pointer to that uninitialized memory to the function. The function in turn uses that memory to create the returned object, avoiding the copy from the returned object to the variable in GPoint p = circle.GetCentre();, performing a single copy from circle.c_Centre to p (or to the unnamed variable if you bound a reference to constant).

David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489
  • I agree with you the copy is performed. But what about the assignment operator? GPoint & Centre = GetCentre() ===> Gpoint & Centre = Copy of point bu there no need for = operator to be called. while GPoint point = GetCentre() ==> GPoint point = Copy of Point ===> Then assignment operator is called ! – Sal Dec 06 '11 at 21:55
  • @Sal Wrong, In `GPoint point = circle.GetCentre();` the syntax *looks* like assignment but is *copy initialization* there is no call to `operator=` anywhere. – David Rodríguez - dribeas Dec 06 '11 at 21:59
  • Which is why you should prefer to write GPoint point(circle.GetCentre()); Then instead of assignment operator taking a temp and making a copy, you get it optimized down to a single creation operation (assuming you wrote everything reasonably correctly) – Mordachai Dec 06 '11 at 22:06
  • To clarify: David is correct if you compiler optimizes it as such (debug builds for me would not be optimized, I would see the assignment copy execute, for example). – Mordachai Dec 06 '11 at 22:07
  • 1
    @Mordachai: This has nothing to do with optimizations, any compiler that calls `operator=` on `GPoint point = circle.GetCentre();` is broken or non-standard. `operator=` is and should only be called when the left-hand-side expression refers to an already existing object, which is not the case as in this case it is creating a new `GPoint` – David Rodríguez - dribeas Dec 06 '11 at 22:15
  • @David ah, perhaps I am purely mistaken. I thought that this broke down as default ctor followed by assignment operator on returned temp. But perhaps I've just misunderstood for years! – Mordachai Dec 06 '11 at 22:21
  • @ David Rodríguez - dribeas. You are totally right, my mistake it is initialization. It was big code and I cut down. – Sal Dec 06 '11 at 22:23
3

No.

It should not even compile in the current state.

circle.GetCentre();

This returns an object.
Since you do not assign it to the variable it is an unamed temporary object.

temporary can not be bound to a reference (though they can be bound to a const refererence).

// This should be a compiler error
GPoint& Centre = circle.GetCentre();

// This should compile
GPoint const& Centre = circle.GetCentre();   

When you bind a temporary to a const reference it's lifespan is extended to the life of the reference.

Martin York
  • 257,169
  • 86
  • 333
  • 562
  • However, this doesn't gain Sal anything - it is bound to a temp copy just as if you'd done GPoint Centre(circle.GetCentre()); – Mordachai Dec 06 '11 at 21:54
  • @Sal: Then you are using an old broken compiler. There are lots of free modern compiler around please upgrade. – Martin York Dec 07 '11 at 00:26
2

No, it's not safe or valid. GCircle::GetCentre() returns-by-value, so the memory where the returned value is temporarily stored will be invalid at the end of the statement. Assigning a piece of data to a reference variable really only stores a pointer to the original's address in memory. Once that memory is invalid, Centre could be referencing any memory of any type, and will blindly treat it like a GPoint.

To save a value returned-by-value, you would need to say GPoint Centre = circle.GetCentre();. If you indeed want a reference to circle's c_Centre member, you should rewrite GetCentre() as such:

GPoint& GetCentre() const {return c_Centre;}

Furthermore, since you probably don't want people outside of circle to change its centre, you should probably return it as a const GPoint&:

const GPoint& GetCentre() const {return c_Centre;}

This will cause anyone looking at the new Centre local variable to think its const, without changing the way circle views the same piece of data.

matthias
  • 2,419
  • 1
  • 18
  • 27
  • "Once that memory is invalid" when this happens? when ButtonClick() is out of scope? I am guessing yes because GPoint destructor called three times when ButtonClick() gets out of scope. The third time correspond to the temporary copy of Centre? – Sal Dec 06 '11 at 21:52
  • 1
    And this is a valid and well understood idiom to use. It leaves it up to the caller to choose a const ref (better performance [no copy], but cannot change the value) or declare a local copy (worse performance, but can manipulate the value or hold it beyond the scope of the source). – Mordachai Dec 06 '11 at 21:55
  • 1
    @Sal as I said at the end of my first sentence, the returned value is a temporary, and is invalid at the end of the statement--in other words, when you hit the semicolon at the end of the assignment. – matthias Dec 06 '11 at 21:59
1

No, it is not valid, GetCentre() returns a temporary and references cannot be bind to rvalues. You can, however, bind it to const reference:

const GPoint& centre = circle.GetCentre();

In this case, the "lifetime" of the rvalue is extended so the const reference remains valid while it is in scope.

jrok
  • 54,456
  • 9
  • 109
  • 141
  • 1
    Yes, but a copy was created here. So there is zero performance improvement. – Mordachai Dec 06 '11 at 21:52
  • @Mordachai It'll be optimized away almost certanly. Check the answer of David Rodriguez. I think OP is optimizing prematurely. (no pun intended there :)) – jrok Dec 06 '11 at 21:58
  • I'm guessing that the OP doesn't have a solid grasp of what references are for, and object lifetimes in general. He should read Scott Meyers. – Mordachai Dec 06 '11 at 22:12
0

There's nothing wrong with returning a reference to a member, just so long as you don't delude yourself that you can use that reference beyond the scope of the class object from which it came.

Ex:

GPoint Point1(10,2);
{
    GCircle circle(Point1, 100);//100 is the radius.

    GPoint & centre = circle;   // valid for lifetime of circle.

    ... // do stuff with centre
}
// centre no longer pointing at valid object, but then, centre is itself out of scope, so no issue!
Mordachai
  • 9,412
  • 6
  • 60
  • 112