2

I have recently learned how to convert a pointer into a reference using this tip. But when I do this in an accessor, it seems to create an undefined behavior, and I can't understand why.

Why I want to do this

I have a class WalkerOwner which owns an instance of the class Walker. This instance of Walker is needed elsewhere in the code, so I have provided an accessor (getter) to the class WalkerOwner, which gives a reference.

class WalkerOwner
{
public:
    ...
    Walker& getWalker() {return m_ownedWalker;} ;
private:
    Walker m_ownedWalker;
};

Later on, I realized that WalkerOwner should actually internally manage a pointer to the walker for some reason. As I don't want to refactor the rest of my code, changing every reference to a pointer, I have tried to convert the pointer into a reference in the getter.

Code which shows the problem

#include <iostream>
#include <string>
using std::string;

class Walker
{
    public:
        Walker() : m_travelledDistance(5) {};
        ~Walker(){};

        void swank() { std::cout << "I have walked " << distanceAsString() << "! How good I am!" <<  std::endl; };

    private:
        // Calling this function makes the result even more impressive
        string distanceAsString() { return std::to_string(m_travelledDistance) + " meters" ; };

        int m_travelledDistance;
};

class WalkerOwner
{
    public:
        WalkerOwner() {m_ownedWalker = new Walker; } ;
        ~WalkerOwner() { delete m_ownedWalker;};

        // I know I should throw an exception if the pointer is not valid.
        Walker& getWalker() {return Walker(*m_ownedWalker);} ; //conversion from pointer into reference

    private:
        Walker* m_ownedWalker;
};


int main(int argc, char *argv[])
{
    // -------
    // Case 1
    // If "main" owns John the walker, everything seems fine.
    // -------
    Walker* ptrToJohn = new Walker ;
    Walker& john = Walker(*ptrToJohn); //conversion from pointer into reference

    john.swank();

    delete ptrToJohn ;

    // -------
    // Case 2
    // If someone else owns Jack the walker, a disaster occurs.
    // -------
    WalkerOwner walkerOwner ;
    Walker& jack = walkerOwner.getWalker() ;

    // (When I put a breakpoint here, my Integrated Devlopment 
    // Environnment says that walker.m_travelledDistance = 5)

    jack.swank(); 

    std::cin.get(); // Press enter to close
    return EXIT_SUCCESS;
}

When I run this code, I get the following output:

I have walked 5 meters! How good I am!

I have walked -858993460 meters! How good I am!

An interesting thing is that a breakpoint shows that jack is in the expected state just before calling the function "swank()". (Humm... Maybe swanking has made him lose his mind!)

Anyway, I would be very glad if someone could explain to me this behavior, and could tell if it is possible to make a safe accessor that performs this conversion.

Community
  • 1
  • 1
Lorèloi
  • 168
  • 1
  • 8
  • You are returning a new instance of `Walker`. Instead just use: `return *m_ownedWalker;` – quamrana Jun 15 '16 at 09:21
  • So you don't like `std::unique_ptr` then? Your `WalkerOwner` class is deeply flawed. The compiler generated copy constructor and assignment operators will give you gyp. – Bathsheba Jun 15 '16 at 09:23
  • Off topic : Walker& john = Walker(*ptrToJohn); this is deep copy not just conversation – Humam Helfawi Jun 15 '16 at 09:23
  • 1
    You didn't follow the tip you learned from; as the answer says, "[all] we have done is de-referenced the pointer to the object". – molbdnilo Jun 15 '16 at 09:31
  • @Bathsheba ; Actually, I have tried both `unique_ptr` and the raw pointer, and I wanted to use `unique_ptr` in the first place. But `unique_ptr` neither seemed to clarify nor solve something in this specific problem, so I felt that I should keep my code as basic as possible. But maybe I was wrong and `unique_ptr` expresses its purpose in a clearer way. – Lorèloi Jun 15 '16 at 10:23
  • Don't do that. Bin this and *learn* to love `std::unique_ptr` and `std::shared_ptr`. Trust me. – Bathsheba Jun 15 '16 at 10:24
  • @molbdnilo ; Yes, it seems I don't actually understand what "de-reference" exactly meant in the tip I have tried to follow... – Lorèloi Jun 15 '16 at 10:32
  • @Bathsheba ; I trust you. :) Is it worth editing my question ? – Lorèloi Jun 15 '16 at 10:34

1 Answers1

5

By writing return Walker(*m_ownedWalker);, you're not "converting" from *m_ownedWalker to a full Walker: you're creating a new Walker using the current one as a source.

Simply return *m_ownedWalker;

As @user2079303 points out: because you're returning a new Walker, and then not saving it anywhere (saving a reference is not the same as saving the actual value), it gets destroyed immediately. And then you derefence the (now-dead, uninitialised-anyway) copy - no wonder garbage was printed out!

John Burger
  • 3,662
  • 1
  • 13
  • 23
  • To add to your answer, I will advice either implementing the copy-constructor and assignment operator, or disallowing those operations setting the definition as private or marking them as deleted if you are using c++11 – Ricardo Amores Jun 15 '16 at 09:30
  • 2
    To expand this a bit: When `getWalker` returns, that new *temporary* `Walker` is destroyed. And the reference to it that was returned from the function is left dangling. Using the destroyed object in any way - including swanking or observing its state - through this dangling reference has undefined behaviour. – eerorika Jun 15 '16 at 09:31
  • @Ricardo I have a far more fundamental issue with a getter method returning the actual object in the first place: might as well make the variable itself `public`! @quamrana! Read what Ricardo wrote! – John Burger Jun 15 '16 at 09:32
  • Well I've used this approach sometimes e.g: I've created a OO Wrapper for some systems in SDL, and when initializing the resources you need to use SDL functions that work with pointers to create/destroy that resource, so I've use a shared_ptr with a custom deleter to manage the lifecycle of the object. However I didn't want to expose a pointer to the rest of the app, so I've just returned a reference instead. Seem like a good idea to me but I can be mistaken :D – Ricardo Amores Jun 15 '16 at 09:38
  • This answer indeed fixes the behavior of the programm and gives some key to understand what is going on (thanks for this!), but user2079303's comment is exactly the explaination I was looking for. I think it would be very nice if it could be added to this answer (or another). :) – Lorèloi Jun 15 '16 at 12:36
  • @JohnBurger ; Is returning an actual object really a bad thing? I mean, if `Walker` doesn't provide any function which is dangerous for its internal state, and if `WalkerOwner` won't be compromised if `m_ownedWalker` changes, is there still an issue somewhere? (I am just trying to understand.) – Lorèloi Jun 15 '16 at 12:59
  • The `WalkerOwner` change from an object to a pointer muddied things a bit - by adding the method he at least got away from the problems of pointers. But why did he use a pointer in the first place? So that he could change the Walker mid-stream? All right, so be it. But he'd _previously_ given complete access to the previous `Walker` to anyone who asked - and they could have cached the value themselves. Now `WalkerOwner` rips that old `Walker` out from under the previous caller - perhaps `delete`ing it! Chaos results. – John Burger Jun 15 '16 at 13:05
  • @Lorèloi Note that he's not returning an object, he's returning a reference to that _exact_ object - not a copy. If you have an object marked `private`, and then you give a non-`const` reference to it, then the object might as well have been public: you've just done the same thing anyway. `WalkerOwner` at least "value added" by converting the pointer to a reference - that's one time when a "getter" is doing its job. – John Burger Jun 15 '16 at 13:07
  • That garbage, however, is somewhat well-defined garbage: `-858993460` is `0xCCCCCCCC` (in hexadecimal display), which is the pattern used by Microsoft's C++ debugging runtime to mark uninitialized stack space (see [Magic number: magic debug values](https://en.wikipedia.org/wiki/Magic_number_(programming)#Magic_debug_values)). – IInspectable Jul 19 '16 at 09:24