-2

I have once asked this question and after hours of research I think the answers are not suitable and do not fix my issue. Please inspect this code:

 #include <iostream>
using namespace std;

class Test
{
public:
    Test();
    Test(int);
    ~Test();
    friend Test Problem();
    int get_a ();
private:
    int* a;
};

Test::Test(int x)
{
    a = new int[1];
    *a = x;
}

Test::~Test()
{
    delete [] a;
}

Test Problem()
{
    Test doomed(1);
    int* doomed_too = new int[1];
    *doomed_too = 99;
    doomed.a = doomed_too;
    return doomed;
}

int Test::get_a()
{
    return *a;
}

int main()
{
    Test Sniffer = Problem();
    if (Sniffer.get_a() == 99) cout << "You saved me! Thanks!";
    return 0;
}

Yes, I know that object doomed will be destroyed before returned to object Sniffer, so Sniffer will get random value from the freed memory of the Heap. My issue is - how to save the "life" of this object doomed? Can you provide working code/"fix"?

Furthermore, I know this specific example has many workarounds but on the project I am working on I am not allowed to do such workarounds (no strings, no vectors, just dynamic array of ints without using further libraries and complications).

I will be glad if you can test your solution first (before answering).

I did lot of research on this topic and I am amused that all the lectures and examples always use not-dynamic private members or if they use in their examples dynamic private members they always "skip" to show operator+ overloading or operator* overloading (just examples).

How can we make operator+ overloading public-member function of a given Class, if the same Class do posses a dynamic private-member - for example int* n?

mirosoft
  • 285
  • 1
  • 2
  • 7
  • 1
    Read http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three – Zan Lynx Apr 08 '14 at 19:36
  • Your question has nothing to do with operator+ as far as I can see. – Zan Lynx Apr 08 '14 at 19:38
  • Your friend class Problem is changing internal values of Test objects without doing it correctly. It is leaking memory by not freeing doomed.a before reassigning its value. And it probably should not be doing that anyway. – Zan Lynx Apr 08 '14 at 19:39
  • Thank you for your answer - can you advice me with fixing this code? I already know the Rule of Three and Rule of Five. Unfortunately, this do not answer my question at all - it just explain why it happens. The original question share the logic with the last question - that's why I added it to this topic. – mirosoft Apr 08 '14 at 19:41
  • `a = new int[1]` should be deleted will `delete [] a`. Use `a = new int` instead (that is correctly deleted with `delete a`). – ChronoTrigger Apr 08 '14 at 19:43
  • @ZanLynx This is just an example of the original issue, which is too long and boring to be passed here. – mirosoft Apr 08 '14 at 19:44
  • @mirosoft: this question contains code so bad that the actual question can't be answered. – Zan Lynx Apr 08 '14 at 19:44
  • @ChronoTrigger This is not necessary to be done in the current issue (it is irrelevant). – mirosoft Apr 08 '14 at 19:45
  • @mirosoft: I think that I recommend adding to your question some code written as a **unit test**. A function that sets up the problem and gets the result then checks it for correctness. That would tell us all exactly what you want the code to do. – Zan Lynx Apr 08 '14 at 19:47
  • @ZanLynx Thanks for your advice - I just did it. Sorry if I am chaotic in my thoughts ... The purpose is to trigger the IF :) – mirosoft Apr 08 '14 at 19:52
  • @mirosoft See it as a piece of advise to save you another question later. – ChronoTrigger Apr 08 '14 at 19:56
  • Turns out that the Rule of Three totally DOES answer your question. Without a copy constructor your code was deleting the value before it could be returned. – Zan Lynx Apr 08 '14 at 22:30

2 Answers2

3

You should implement a copy constructor:

Test::Test(const Test& other)
{
  this->a = new int;
  *this->a = *other.a;
}

Some other problems:

  • why you create a = new int[1]? it should be just a = new int

  • if you really need a = new int[1] the corresponding delete for arrays in C++ should usually be delete [] a.

Alternatively, you can use a smart pointer (std::tr1::shared_ptr or similar, depending on your compiler), see http://en.cppreference.com/w/cpp/memory/shared_ptr

Andrei Bozantan
  • 3,781
  • 2
  • 30
  • 40
0

Without a custom copy constructor and copy assignment operator, you are creating multiple objects that point to the same memory. That's the source of your problem.

Whenever you have a class that allocates memory for its member data in its constructor, you also need to implement the copy constructor and copy assignment operator. Please read The Rule of Three.

Community
  • 1
  • 1
R Sahu
  • 204,454
  • 14
  • 159
  • 270