I think this is bad C coding ('new' in a function to point to a local variable). Am I co-CodePudding

I want to create a class that contains a pointer, and upon initialization, the pointer can be dereferenced to give an integer assigned at initialisation.

This was my first attempt to write this code. This passed compiler and gave me the correct result without warning. However I later think this code has a potential problem.

That is in the constructor, the integer a is created on a stack framed to the constructor function. I am however making 'ptr' pointing to this stack memory address. The problem is this memory can be reused upon maybe calling other functions, so I might get garbage value if I am not lucky.

#include <iostream>
using namespace std;


class P {
  public:
    int *ptr;
    P(int);
};

P::P(int a){
  int *ptr = new int (0);
  ptr = &a;
}

int main() {
  P p(99);

  cout <<*(p.ptr) <<endl;

}

A better way would be to create an integer variable on heap, copy the value of a to that variable, and make ptr pointing to that memory space on heap.

P::P(int a){
  int *i = new int (0);
  *i = a;
  ptr = i;
}

Is my analysis correct?

Thanks!

CodePudding user response：

The statements

int *ptr = new int (0);
ptr = &a;

are problematic, but probably not because of the reasons you think.

The reason it's problematic is because you define a new and distinct local variable ptr inside the function. This is not the same as the P::ptr member variable.

There's also a second problem, which is a memory leak. You allocate memory with new, but you never free the memory with delete.

If you really is required to use a raw and non-owning pointer to a single int value, I recommend you do it using a constructor initializer list:

P(int a)
  : ptr{ new int(a) }
{
  // Empty
}

Here new int(a) will create a new int value and copy the value of a into it.

Remember to then create a destructor which free's the memory you have allocated. And then you need to learn about the rules of three, five and zero.

To use the rule of zero, and to avoid memory leaks and make life simpler, use a smart pointer like std::unique_ptr:

struct P
{
  std::unique_ptr<int> ptr;

  P(int a)
    : ptr{ std::make_unique<int>(a) }
  {
  }
};

And of course, since it's just about a single int value, and you don't need a reference to the original value, there's no need for pointers at all.