2

I'm experiencing some weird behavior and I'm not really sure where to turn.

Basically, I have a set of classes and one of them should be constructed with instances of the other two. I'm using pass by reference to assign the resources but the second assignment is crashing on my machine. I don't understand why the second assignment crashes but the first works fine. To make this a little more confusing, I tried recreating the problem in an online cpp compiler, but it seems to run fine in that environment.

I obscured the class names and removed a few methods that didn't seem relevant to this problem. Does anyone have any ideas?

#include <iostream>

using namespace std;

class Driver{};
class ITransmission{};
class ManualTransmission : public ITransmission {};

class Car
{
public:
    Car(ITransmission &trans, Driver &driver);

private:
    ITransmission *m_trans;
    Driver *m_driver;
};

Car::Car(ITransmission &trans, Driver &driver)
{
    *m_trans = trans;
    *m_driver = driver; // <-- **** Crashes here!?!? ****
}

int main()
{
    ITransmission *trans = new ManualTransmission();
    Driver *driver = new Driver();
    Car car(*trans, *driver);

    return 0;
}
eeScott
  • 133
  • 9
  • 3
    `*m_driver` means "go to the address stored in `m_driver` and get me the value there." Now ask yourself: where is `m_driver` pointing when you try to dereference it there? – scohe001 Feb 17 '20 at 21:54
  • 1
    m_trans and m_driver dont point to anything, your code has UB. it has nothing to do with passing by reference. – Borgleader Feb 17 '20 at 21:54
  • 1
    Thank you for a clear question with a proper demonstrating example! I won't answer because people have already answered in comments. BTW it's mentioned [here](https://en.wikipedia.org/wiki/Has-a#C++) that your toy model doesn't need pointers; probably your real code doesn't need them too. – anatolyg Feb 17 '20 at 21:56
  • @Borgleader That makes sense, I was thinking it would work because it's assigning the dereferenced value, but I guess that doesn't really make sense now that I think about it. So if I want to construct Car with pre-initialized Driver and Transmission objects, am I going to need to use something like `Car(ITransmission *trans, Driver *driver);` then assign the members using `m_trans = trans`? – eeScott Feb 17 '20 at 21:59
  • @eeScott What's the expected lifetime of the `ITransmission` and `Driver` objects within the `Car` object? Could they ever be moved to another `Car`? Or do they exist for the entire lifetime of the `Car`, and only for this `Car`? – JohnFilleau Feb 17 '20 at 22:17

1 Answers1

1

*m_trans and *m_drivers are only pointers to their respective types. They are not the object themselves. In your copy constructor you are calling 

*m_trans = trans

which crashes because m_trans doesn't point to anything and you are dereferencing it. What you want instead is,

m_trans = &trans
m_driver = &driver

This sets the pointer to point at the address of the object you've passed in, whereas before you were trying to assign to the pointed-to object.

LLSv2.0
  • 401
  • 4
  • 15
  • Is there any advantage to doing what you suggest over changing the constructor's signature to `Car(ITransmission *trans, Driver *driver);` then assign the members using `m_trans = trans`? – eeScott Feb 17 '20 at 22:07
  • 2
    @eeScott the two are exactly the same. However, note that if those pointers or references you pass in become invalid, you'll be right back into Undefined Behavior land again. I'd strongly suggest having `Car` do its own memory management. – scohe001 Feb 17 '20 at 22:10
  • 1
    Not really. I saw your comment after I had written my answer. You're now getting into the realm of what is "more readable" to the user. There are times when people prefer pass by pointer and times when people prefer pass by reference. I suppose there is a slight advantage in your specific case to use pass by pointer so that you don't have to dereference your pointers when you call the function, but that's starting to get nitpicky. – LLSv2.0 Feb 17 '20 at 22:11
  • I tried both solutions and they seem to work the same, so I think it's possible. I actually started going down this path because I wanted to be able to stub out transmission and driver for unit testing purposes. If I have `Car` doing it's own memory management, I'm not aware of a way I can stub out those members. – eeScott Feb 17 '20 at 22:14
  • @eeScott is there a reason you need pointers at all? From the small amount of code you've shown us, I can tell you it's extremely likely to bite you again. – scohe001 Feb 17 '20 at 22:16
  • 1
    @eeScott Your use-case probably calls for using the [pimpl idiom](https://stackoverflow.com/a/8972757/509868). I use it all the time. It has one little wrinkle though, described [here](https://stackoverflow.com/q/9020372/509868). – anatolyg Feb 17 '20 at 22:18
  • @scohe001 The only real reason is that I wanted to do pass by reference so I could have a factory class create everything using the same instances - `Transmission` and `Driver` get used elsewhere and there's no reason to have duplicate instances (actually it might cause issues in my use case). – eeScott Feb 17 '20 at 22:18