Can anybody explain me why this code is not working?

Question

I'm learning C++. Now, I'm trying to make one sample related with overloading operators of an object. My object (called Contador) has different methods and variables which help user to count iterations.

Header file of the object:

class Contador
{
private:
    int* Valor;
    int* Salto;

public:
    Contador(int Valor_Inicio = 0, int Salto = 1);
    ~Contador();

inline int Get_Valor() const { return *Valor; }
inline int Get_Salto() const { return *Salto; }

inline void Incremento() { Set_Valor(Get_Valor() + Get_Salto()); }

inline void operator++ () { Set_Valor(Get_Valor() + Get_Salto()); }

void Set_Valor(int Valor);
void Set_Salto(int Salto);

};

Cpp file of the object:

// Librerias Propias
#include "Contador.h"

Contador::Contador(int Valor_Inicio, int Salto)
{
    Set_Valor(Valor_Inicio);
    Set_Salto(Salto);
}

Contador::~Contador()
{
    delete Contador::Valor;
    delete Contador::Salto;
}

void Contador::Set_Valor(int Valor)
{
    delete Contador::Valor;
    Contador::Valor = new int(Valor);
}

void Contador::Set_Salto(int Salto)
{
    delete Contador::Salto;
    Contador::Salto = new int(Salto);
}

The main() function of the sample has 2 different for loops. In the first one, I call Incremento() method and in the second one I call the overloaded operator.

Main function:

void main()
{
    // Genero el elemento de analisis.
    Contador* contador = new Contador();

    // Realizo el bucle con la función de incremento.
    std::cout << "Incremento()" << std::endl;
    for (contador->Set_Valor(0); contador->Get_Valor() < 3; contador->Incremento())
    {
        // Escribo algo.
        std::cout << "Iteracion actual: " << contador->Get_Valor() << std::endl;
    }

    // Realizo el bucle on el operador sobrecargado
    std::cout << "operador sobrecargado" << std::endl;
    for (contador->Set_Valor(0); contador->Get_Valor() < 3; contador++)
    {
        // Escribo algo.
        std::cout << "Iteracion actual: " << contador->Get_Valor() << std::endl;
    }
}

The problem appears when main function passes the first iteration of the second loop. It throws one exception in Get_Valor() method.

It seems to me that it change the memory addres of the pointer Valorin some place, but I can`t find where.

Can anybody help me? Thanks.

You can rewrite this code **without using any pointers at all**. — john, Aug 29 '19 at 08:11
In the constructor, you delete an uninitialized pointer, which is undefined behavior. — Daniel Langr, Aug 29 '19 at 08:17
There are two errors in this code, both related to pointers (see the other posts and comments). There is absolutely no need to use pointers in this class, you are just making things difficult for yourself. It seems you could benefit from reading a [good book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) on C++ which will give you a more balanced introduction to C++. — john, Aug 29 '19 at 08:20
@john I know I can rewrite this code without using pointer. However, I want to use pointers in order to use them correctly. — , Aug 29 '19 at 08:24
Not using pointers when they are not needed *is* using them correctly. As a reasonable approximation, `new` should not appear in code you write. — Caleth, Aug 29 '19 at 08:31
Another undefined behavior: if allocation fails, you delete a pointer that has not been allocated. Example: `{ Contador c; c.Set_Valor(1); /* what if new fails? */ }`. Maybe, you want to use `std::unique_ptr` instead if you insist on pointers. — Daniel Langr, Aug 29 '19 at 08:31
@pH4ngH0st Well a third error concerning pointer use is that you are not following the [rule of three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). Pointers are difficult, and in modern C++ not particularly useful. There might be better areas for you to concentrate on. — john, Aug 29 '19 at 09:49

molbdnilo · Accepted Answer · 2019-08-29T08:31:41.203

3

contador++ does not do what you think it does - contador is a pointer, not a Contador, so it will make contador point to something that does not exist.
You need to dereference the pointer.

However, *contador++ also increments contador - it is *(contador++) - and (*contador)++ does not compile because you have only overloaded the prefix operator (the postfix operator has the prototype operator++(int).

So, ++*contador will do what you want.

You can avoid many similar problems, and the clunky syntax, by not using pointers unnecessarily.

edited Aug 29 '19 at 08:31

answered Aug 29 '19 at 08:16

molbdnilo

64,751
3
43
82

Excellent answer! You even answered the answers' problems, and the problems of the comments to the answer's answers! – Adrian Mole Aug 29 '19 at 08:32

Adrian Mole · Answer 2 · 2019-08-29T08:25:40.800

2

The expression contador++ increments the address that contador (a pointer) points to! So, after the first iteration, the pointer will be completely invalid.

To call the increment operator, you need: ++(*contador) which first dereferences the pointer to the object pointed to, then effects that object's increment operator.

edited Aug 29 '19 at 08:25

answered Aug 29 '19 at 08:14

Adrian Mole

49,934
160
51
83

1

Since he's new to c++ may be more clear to read `(*contador)++` – Moia Aug 29 '19 at 08:18

score 2 · Answer 3 · answered Aug 29 '19 at 08:32

3 coding issues:

main shoudl return int.
Valor and Salto are not initialized in constructor. Contador::Set_Valor and Contador::Set_Salto requires initialized pointers (as you delete them).

Easy fix is:
```
class Contador
{
private:
    int* Valor = nullptr;
    int* Salto = nullptr;
//...
};
```
Last issue is in your last loop:
```
for (contador->Set_Valor(0); contador->Get_Valor() < 3; contador++)
```
As condator is a pointer (not pointing on an array), accessing condator[1] would be UB. You wanted ++(*condator) (operator++ () is pre-increment whereas operator++ (int) is post-increment).

Finally, avoiding usage of all those pointers would simplify code (and no bother with rule of 3 you break):

class Contador
{
private:
    int Valor;
    int Salto;

public:
    Contador(int Valor = 0, int Salto = 1) : Valor(Valor), Salto(Salto) {}
    ~Contador() = default;

    int Get_Valor() const { return Valor; }
    int Get_Salto() const { return Salto; }

    void Incremento() { Set_Valor(Get_Valor() + Get_Salto()); }
    void operator++ () { Set_Valor(Get_Valor() + Get_Salto()); }

    void Set_Valor(int Valor) { this->Valor = Valor;}
    void Set_Salto(int Salto) { this->Salto = Salto;}
};

int main()
{
    Contador contador;

    std::cout << "Incremento()" << std::endl;
    for (contador.Set_Valor(0); contador.Get_Valor() < 3; contador.Incremento())
    {
        std::cout << "Iteracion actual: " << contador.Get_Valor() << std::endl;
    }

    std::cout << "operador sobrecargado" << std::endl;
    for (contador.Set_Valor(0); contador.Get_Valor() < 3; ++contador)
    {
        std::cout << "Iteracion actual: " << contador.Get_Valor() << std::endl;
    }
}

There is another issue as well. If `new` fails in `Set_Valor` that has not been called from a constructor, then `delete` in the destructor (or, subsequent `Set_Valor`) will cause UB. Just to make your answer complete. — Daniel Langr, Aug 29 '19 at 08:33
@DanielLangr: In OP's case, `contador`'s destructor isn't called as it leaks, so it is not an "issue" ;-) (good catch BTW). — Jarod42, Aug 29 '19 at 08:39

Boris Lyubimov · Answer 4 · 2019-08-29T09:42:10.423

In addition to previous answers. As I could see Contador* contador = new Contador(); code also contains UB (undefined behaviour) This call is equal to constructor with parameters Contador(0, 1) which will do Set_Valor and Set_Salto which call delete first but at this moment content of this variables is not guaranteed to be nullptr so you might corrupt data. Also compiler if it sees UB might optimize out all other code since it's already UB and it can change behaviour anyway it wants for example throw it away completely. https://devblogs.microsoft.com/oldnewthing/20140627-00/?p=633

Can anybody explain me why this code is not working?

4 Answers4