2

I have a class named HighWaterDetector:

class HighWaterDetector
{
public:
    HighWaterDetector(Device* device);
    Device * devicePtr;
    Output * output1Ptr;
    CloudMsgParser * cloudMsgParserPtr;
    Output output1;
    NCD2Relay ncd2Relay;
    CloudMsgParser cloudMsgParser;
};

with constructor:

HighWaterDetector::HighWaterDetector(Device* device): ncd2Relay(), output1(1, &ncd2Relay){
}

I am trying to initialize an instance of Output in the member initialization list for HighWaterDetector but Output which requires you to pass a pointer to an instance of NCD2Relay which is also a member of the class HighWaterDetector. My program crashes inside the output constructor. Is this the wrong way of doing this? What am I doing wrong?

Output class:

class Output
{
public:
    Output(ushort relayNum, NCD2Relay* ncd2RelayPtr);
    ushort relayNum;
    OutputStatus outputStatus;
    int setOutputOn(void);
    int setOutputOff(void);
    void process(void);
    NCD2Relay* ncd2RelayPtr;
};


//Output Constructor
Output::Output(ushort relayNum, NCD2Relay* ncd2RelayPtr) {
    this->relayNum = relayNum;
    this->ncd2RelayPtr = ncd2RelayPtr; //DOESNT CRASH IF I COMMENT THIS OUT
    this->outputStatus.outFail = 0;
    Serial.print("Initializing output ");
    Serial.println(this->relayNum);
    this->setOutputOff();
}
Felix
  • 51
  • 2
  • 10
  • Can't you initialize this variable _in_ the constructor? – ForceBru Aug 10 '16 at 21:09
  • What's wrong? Wrong design. What are you trying to archieve with that cyclic dependency. Is it really necessary to have these dependencies? – KIIV Aug 10 '16 at 21:10
  • I don't think this should be a problem unless the `Output` constructor dereferences `ncd2RelayPtr`. – molbdnilo Aug 10 '16 at 21:30
  • @KIIV is there a better solution to doing this? – Felix Aug 10 '16 at 21:50
  • If you name the inputs to the constructor something different than the class members (like `inNcd2RelayPtr`) then you don't need to use `this` (just `ncd2RelayPtr = inNcd2RelayPtr;`), and you can be more sure that your code is behaving properly. – Keith M Aug 10 '16 at 21:50

3 Answers3

4

Did you heed to your compiler warnings? or are they turned on to maximum? The order of declaration of your members may be the cause:

class HighWaterDetector
{
public:
    HighWaterDetector(Device* device);
    Device * devicePtr;
    Output * output1Ptr;
    CloudMsgParser * cloudMsgParserPtr;
    Output output1;                     // <- This is constructed before
    NCD2Relay ncd2Relay;                // <- This...
    CloudMsgParser cloudMsgParser;
};

but your constructor goes like:

HighWaterDetector::HighWaterDetector(Device* device): ncd2Relay(), output1(1, &ncd2Relay){ ... }

In the above context, using the address of ncd2Relay in the constructor of output1 is simply using a pointer to an uninitialized object which is Undefined Behavior when you eventually dereference it before it's construction. Hence, You will need to enforce the ordering in your class definition...

Just to quote from the C++ standard: (emphasis mine) [class.base.init/13]

In a non-delegating constructor, initialization proceeds in the following order:

  • First, and only for the constructor of the most derived class ([intro.object]), virtual base classes are initialized in the order they appear on a depth-first left-to-right traversal of the directed acyclic graph of base classes, where “left-to-right” is the order of appearance of the base classes in the derived class base-specifier-list.

  • Then, direct base classes are initialized in declaration order as they appear in the base-specifier-list (regardless of the order of the mem-initializers).

  • Then, non-static data members are initialized in the order they were declared in the class definition (again regardless of the order of the mem-initializers).

  • Finally, the compound-statement of the constructor body is executed.

WhiZTiM
  • 21,207
  • 4
  • 43
  • 68
  • Program still crashes when declaring an instance of HighWaterDetector. – Felix Aug 10 '16 at 21:26
  • @Felix, have you reordered the declarations of `output1` and `ncd2Relay` in your class definition? – WhiZTiM Aug 10 '16 at 21:31
  • Yes crashes actually in the Output constructor. See above... I posted that constructor as well. – Felix Aug 10 '16 at 21:36
  • 1
    @Felix, then you actually have an extra bug other than your ordering problem. Have you step through your code with the debugger? – WhiZTiM Aug 10 '16 at 21:53
3

In C++ the initializer list doesn't follow the order in which you write the items in the initializer. It rather follows the order in which the members are declared in your class. Since you put your NCD2Relay after your Output inside the class, then NCD2Relay will be initialized after Output, even though in the initializer you put NCD2Relay first. So, just move NCD2Relay before the Output inside your class declaration.

grigor
  • 1,584
  • 1
  • 13
  • 25
  • You beat me to it. That is in fact the answer. Compilers today throw errors for those things. – Giora Guttsait Aug 10 '16 at 21:11
  • I reordered them now. Do you know why its crashing inside the Output constructor? – Felix Aug 10 '16 at 21:40
  • @Felix It's crashing because `&ncd2Relay` is not defined until the body of the `HighWaterDetector` constructor starts (as opposed to still being in the initializer list for `HighWaterDetector`), for the reason grigor stated; i.e. the constructor of `Output` is being called before `ncd2Relay` is constructed, so the reference of `ncd2Relay` is probably garbage. – Keith M Aug 10 '16 at 21:44
  • @Keith M I modified my class to have NCD2Relay before Output just like grigor said. Are you telling me ncd2Relay is still not defined when the constructor of Output is being called? How do I fix this problem? Is there a different way to do this? Thanks for you help. – Felix Aug 10 '16 at 21:50
  • @Felix I imagine that would have fixed the problem. If not, try my comment on the OP? – Keith M Aug 10 '16 at 21:54
0

You want to take a copy of a pointer you do not control when it gets initialised. I would try to take a pointer to the pointer so even if the original pointer is initialised later you have from this time the correct value.

Flk157
  • 1