Creating a second instance of an object changes whole class behavior (C++)

Question

I have written an Arduino library in C++ that contains an iterator class. If I iterate through it using the same instance all the time, it works as expected. If I create a second instance to do so, it will double the amount of stored objects.

WayPointStack wps = *(new WayPointStack());
wps.AddWP(1, 20);
wps.AddWP(2, 420);

WPCommand c1 = wps.GetNextWP(); //  Stack length: 2, correct
c1 = wps.GetNextWP();           //

WPCommand c1 = wps.GetNextWP(); //  Stack length: 4, not correct
WPCommand c2 = wps.GetNextWP(); //


  WPCommand WayPointStack::GetNextWP()
{
    Serial.println("Pointer = ");
    Serial.println(pointer);
    Serial.println("Length = ");
    Serial.println(_length);
    if (pointer < _length){
        pointer++;
        return _wp[pointer-1];
    }
    return *(new WPCommand(_END, 10000));
}

void WayPointStack::AddWP(int target, int time)
{
    if (_length == arrSize)
        return;
    _wp[_length] = *(new WPCommand(target, time));
    _length++;
}

WayPointStack::WayPointStack()
{
  _wp = new WPCommand[arrSize];
  _length = 0;
  pointer = 0;
}

WPCommand::WPCommand(int target, int time)
{
    _target = target;
    _time = time;
}

Can someone explain this to me?

`WayPointStack wps = *(new WayPointStack());` Wonderful! (sarcasm). Please, read this: https://stackoverflow.com/q/6500313/5470596 — YSC, Mar 27 '19 at 13:19
Sounds like a possible rule of three violation, or a bug in your special member functions. Please provide a [mcve] — NathanOliver, Mar 27 '19 at 13:20
`WayPointStack wps = *(new WayPointStack());` What's wrong with simple `WayPointStack wps;`? — Algirdas Preidžius, Mar 27 '19 at 13:22
@NathanOliver indeed, if `WayPointStack::~WayPointStack()` `delete`s `_wp`, there's UB right here `WayPointStack wps = *(new WayPointStack());`. (well, there's potential for UB) — YSC, Mar 27 '19 at 13:22
oh, that code comes from an other question : https://stackoverflow.com/questions/55375221/parsing-array-in-library-constructor-pointer-problem-c — bruno, Mar 27 '19 at 13:27
Those replacements indeed fixed the problem. Thank you all so much. — Kruspe, Mar 27 '19 at 13:28
I think it's **hiding** the problem. Read about the [rule of zero/three/fire](https://en.cppreference.com/w/cpp/language/rule_of_three) — YSC, Mar 27 '19 at 13:29
@YSC The rule of fire is a mythical and untamed beast. Be careful as you proceed. — super, Mar 27 '19 at 13:32

score 2 · Accepted Answer · edited Mar 27 '19 at 17:27

WayPointStack wps = *(new WayPointStack());

must be

WayPointStack wps;

because it is enough and that removes the memory leak

In

 WPCommand WayPointStack::GetNextWP()
 {
     ...
     return *(new WPCommand(_END, 10000));
 }

you create an other memory leak, may be do not return the element but its address allowing you to return nullptr on error ?

 /*const ?*/ WPCommand * WayPointStack::GetNextWP()
 {
     Serial.println("Pointer = ");
     Serial.println(pointer);
     Serial.println("Length = ");
     Serial.println(_length);
     if (pointer < _length){
       return &_wp[pointer++];
     }
     return nullptr;
 }

else use a static var :

  WPCommand WayPointStack::GetNextWP()
  {
      ...
      static WPCommand error(_END, 10000);
      return error;
  }

In

void WayPointStack::AddWP(int target, int time)
{
    if (_length == arrSize)
        return;
    _wp[_length] = *(new WPCommand(target, time));
    _length++;
}

you create an other memory leak, you just need to initialize the entry :

 void WayPointStack::AddWP(int target, int time)
 {
     if (_length == arrSize)
         return;
     _wp[_length]._target = target, time));
     _wp[_length]._time = time;
     _length++;
 }

you do not signal the error when you cannot add a new element, what about to return a bool valuing false on error and true when you can add :

 bool WayPointStack::AddWP(int target, int time)
 {
     if (_length == arrSize)
         return false;
     _wp[_length]._target = target;
     _wp[_length]._time = time;
     _length++;
     return true;
 }

Finally Why do you not use a std::vector for _wp

It is not really an error, it is expected behaviour. Thank you so much. Really. — Kruspe, Mar 27 '19 at 13:51

score 0 · Answer 2 · answered Mar 27 '19 at 13:37

It looks like you have a memory leak on this line:

return *(new WPCommand(_END, 10000));

It looks like you are creating WPCommand on heap, then throw away pointer and return a copy !!!

The example is not minimal and complete so it is hard to give better pointers.

Creating a second instance of an object changes whole class behavior (C++)

2 Answers2

Linked