Leak or Crash. Don't understand why

[Luk Jack]

OnInitialDialog()...
/// Test cMath
double ans = 1.0f;
cMath *m = new cMath();

ans = m->Power(3,2);
Formula fTest;
fTest.resize(2);
fTest[0].coefficient = 2;
fTest[0].degree = 3;

fTest[1].coefficient = 2;
fTest[1].degree = 1;

m->Derive(fTest);

fTest = m->getFormula();

if (m) {
delete m; // Leak (if absent) or Crash (if present)
m = NULL;
}

[Ulrich Eckhardt]

Post by Luk Jack
/// Test cMath
double ans = 1.0f;
cMath *m = new cMath();
ans = m->Power(3,2);
Formula fTest;
fTest.resize(2);
fTest[0].coefficient = 2;
fTest[0].degree = 3;
fTest[1].coefficient = 2;
fTest[1].degree = 1;
m->Derive(fTest);
fTest = m->getFormula();
if (m) {
delete m; // Leak (if absent) or Crash (if present)
m = NULL;
}

Several points here:
1. Wrapping something into a class that only contains functions and no
actual state (which is what your cMath class looks like, though I'm not
sure) is wrong. If you need, use a namespace instead. I guess you have this
habit and probably a few others from Java or similar languages that force
you to use classes for everything.
2. Don't dynamically allocate things unless you have a good reason to. Just
create a local instance of class cMath if you need.
3. If you really need dynamically allocated objects, use a "smart pointer"
to automatically delete them - C++ doesn't have a garbage collector.
std::auto_ptr would be one, the other would be std::tr1::shared_ptr or
boost::shared_ptr. Note that the former guarantees exclusive ownership
while the other models shared ownership.
4. You don't have to check a pointer before invoking delete on it.

Summary: The problem is in the code you don't show.

Uli

[Luk Jack]

Dear Ulrich,

Post by Ulrich Eckhardt

Post by Luk Jack
/// Test cMath
double ans = 1.0f;
cMath *m = new cMath();
ans = m->Power(3,2);
Formula fTest;
fTest.resize(2);
fTest[0].coefficient = 2;
fTest[0].degree = 3;
fTest[1].coefficient = 2;
fTest[1].degree = 1;
m->Derive(fTest);
fTest = m->getFormula();
if (m) {
delete m; // Leak (if absent) or Crash (if present)
m = NULL;
}

1. Wrapping something into a class that only contains functions and no
actual state (which is what your cMath class looks like, though I'm not
sure) is wrong. If you need, use a namespace instead. I guess you have this
habit and probably a few others from Java or similar languages that force
you to use classes for everything.
2. Don't dynamically allocate things unless you have a good reason to. Just
create a local instance of class cMath if you need.
3. If you really need dynamically allocated objects, use a "smart pointer"
to automatically delete them - C++ doesn't have a garbage collector.
std::auto_ptr would be one, the other would be std::tr1::shared_ptr or
boost::shared_ptr. Note that the former guarantees exclusive ownership
while the other models shared ownership.
4. You don't have to check a pointer before invoking delete on it.
Summary: The problem is in the code you don't show.
Uli
--
C++ FAQ:http://parashift.com/c++-faq-lite
Sator Laser GmbH
Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932

cMath m;
double ans = 1.0f;

ans = m.Power(3,2);

Formula fTest;
Terms t;

t.coefficient = 2;
t.degree = 3;
fTest.push_back(t);


t.coefficient = 2;
t.degree = 1;
fTest.push_back(t);


m.Derive(fTest);

Formula result;
result = m.getFormula();

Changed according to your suggestions. But this time I have heap
corruption. When I comment out this block of code,
everything runs normally. Any further advice please?
Thanks in advance
Jack

[Victor Bazarov]

Post by Luk Jack
Dear Ulrich,

Post by Ulrich Eckhardt
[..]
Summary: The problem is in the code you don't show.

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Did you miss this? If you want your problem solved, post the complete
compilable code that exhibits the problem. Otherwise we are forced to
make guesses.

Post by Luk Jack

Post by Ulrich Eckhardt
Uli

cMath m;
double ans = 1.0f;
ans = m.Power(3,2);
Formula fTest;
Terms t;
t.coefficient = 2;
t.degree = 3;
fTest.push_back(t);
t.coefficient = 2;
t.degree = 1;
fTest.push_back(t);
m.Derive(fTest);
Formula result;
result = m.getFormula();
Changed according to your suggestions. But this time I have heap
corruption. When I comment out this block of code,
everything runs normally. Any further advice please?

Yes. Get rid of all dynamic memory management in your class[es].
You're screwing up somewhere, most likely by violating the "Rule of
Three" (google it). Heap corruption is often caused by deleting the
same pointer twice.

The code you posted here (and in your original post) does not contain
the actual error directly, it's most likely in the guts of the classes
'Formula', 'cMath', 'Terms'.

V

[Luk Jack]

Post by Ulrich Eckhardt

Post by Luk Jack
/// Test cMath
double ans = 1.0f;
cMath *m = new cMath();
ans = m->Power(3,2);
Formula fTest;
fTest.resize(2);
fTest[0].coefficient = 2;
fTest[0].degree = 3;
fTest[1].coefficient = 2;
fTest[1].degree = 1;
m->Derive(fTest);
fTest = m->getFormula();
if (m) {
delete m; // Leak (if absent) or Crash (if present)
m = NULL;
}

1. Wrapping something into a class that only contains functions and no
actual state (which is what your cMath class looks like, though I'm not
sure) is wrong. If you need, use a namespace instead. I guess you have this
habit and probably a few others from Java or similar languages that force
you to use classes for everything.
2. Don't dynamically allocate things unless you have a good reason to. Just
create a local instance of class cMath if you need.
3. If you really need dynamically allocated objects, use a "smart pointer"
to automatically delete them - C++ doesn't have a garbage collector.
std::auto_ptr would be one, the other would be std::tr1::shared_ptr or
boost::shared_ptr. Note that the former guarantees exclusive ownership
while the other models shared ownership.
4. You don't have to check a pointer before invoking delete on it.
Summary: The problem is in the code you don't show.
Uli
--
C++ FAQ:http://parashift.com/c++-faq-lite
Sator Laser GmbH
Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932

Actually, I've got an attribute in my cMath class, so isn't it
stateless, is it? not too sure :)
Thanks
Jack

[Ulrich Eckhardt]

Post by Luk Jack

Post by Ulrich Eckhardt
1. Wrapping something into a class that only contains functions and no
actual state (which is what your cMath class looks like, though I'm not
sure) is wrong. If you need, use a namespace instead. I guess you have
this habit and probably a few others from Java or similar languages that
force you to use classes for everything.

[...]

Post by Luk Jack
Actually, I've got an attribute in my cMath class, so isn't it
stateless, is it? not too sure :)

Yes, that would be a valid reason to put it into a class.

Uli

[Luk Jack]

Post by Ulrich Eckhardt

Post by Luk Jack

Post by Ulrich Eckhardt
1. Wrapping something into a class that only contains functions and no
actual state (which is what your cMath class looks like, though I'm not
sure) is wrong. If you need, use a namespace instead. I guess you have
this habit and probably a few others from Java or similar languages that
force you to use classes for everything.

[...]

Post by Luk Jack
Actually, I've got an attribute in my cMath class, so isn't it
stateless, is it? not too sure :)

Yes, that would be a valid reason to put it into a class.
Uli
--
C++ FAQ:http://parashift.com/c++-faq-lite
Sator Laser GmbH
Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932

class cMath
{
public:
cMath() { }
~cMath() { }
double Integrate(const Formula& f);
void Derive(const Formula& f);
Terms *Solve_Term(const Terms& t);
double Power(double x, int p);
// Formula m_vNomralFunctions; // Normal Functions for generating
approx function
const Formula& getFormula() { return m_vFormula; }
private:
Formula m_vFormula;


};

inline void cMath::Derive(const Formula& f)
{

Terms *t1 = new Terms();
for (unsigned int i = 0; i < f.size(); i++)
{
t1 = Solve_Term(f[i]);
m_vFormula.push_back(*t1);
}
delete t1;

}

inline Terms *cMath::Solve_Term(const Terms& t)
{
Terms *t1 = new Terms();
t1->coefficient = t.coefficient;
t1->degree = t.degree-1;
t1->coefficient = t1->coefficient*t.degree;

if (t1->degree < 0)
t1->coefficient = 0;

//delete t1;
return t1;

}

Thanks

[Luk Jack]

Post by Ulrich Eckhardt

Post by Ulrich Eckhardt

Post by Luk Jack

Post by Ulrich Eckhardt
1. Wrapping something into a class that only contains functions and no
actual state (which is what your cMath class looks like, though I'm not
sure) is wrong. If you need, use a namespace instead. I guess you have
this habit and probably a few others from Java or similar languages that
force you to use classes for everything.

[...]

Post by Luk Jack
Actually, I've got an attribute in my cMath class, so isn't it
stateless, is it? not too sure :)

Yes, that would be a valid reason to put it into a class.
Uli
--
C++ FAQ:http://parashift.com/c++-faq-lite
Sator Laser GmbH
Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932

class cMath
{
        cMath() { }
        ~cMath() { }
        double Integrate(const Formula& f);
        void Derive(const Formula& f);
        Terms *Solve_Term(const Terms& t);
        double Power(double x, int p);
//      Formula m_vNomralFunctions; // Normal Functions for generating
approx function
        const Formula& getFormula() { return m_vFormula; }
        Formula m_vFormula;
};
inline void cMath::Derive(const Formula& f)
{
        Terms *t1 = new Terms();
        for (unsigned int i = 0; i < f.size(); i++)
        {
                t1 = Solve_Term(f[i]);
                m_vFormula.push_back(*t1);
        }
        delete t1;
}
inline Terms *cMath::Solve_Term(const Terms& t)
{
        Terms *t1 = new Terms();
        t1->coefficient = t.coefficient;
        t1->degree = t.degree-1;
        t1->coefficient = t1->coefficient*t.degree;
        if (t1->degree < 0)
                t1->coefficient = 0;
        //delete t1;
        return t1;
}
Thanks

// raise x to the power of p
inline double cMath::Power(double x, int p)
{

double x1 = x;
for (int i = 1; i < p; i++)
x1 *= x;

return x1;

}

[Ulrich Eckhardt]

Luk Jack wrote:
[ snipped former unrelated mail with everything including the signature ]

Post by Luk Jack
// raise x to the power of p
inline double cMath::Power(double x, int p)
{
double x1 = x;
for (int i = 1; i < p; i++)
x1 *= x;
return x1;
}

Firstly, this function is wrong. Even if you restrict yourself to whole,
non-negative numbers, which isn't typically given, at least make sure that
Power(3.14, 0)==1.

Secondly, there is no reason to pass the "this" pointer to this function,
i.e. call this in the context of an object, since it doesn't use the state
of that object. You could make it a class-static function, you could also
make it a function at namespace scope (which includes global scope).

Lastly, don't bother fixing this, because the functionality you need is
provided by the standard library, the function is called pow(). :^)

Uli

[Luk Jack]

Post by Ulrich Eckhardt
[ snipped former unrelated mail with everything including the signature ]

Post by Luk Jack
// raise x to the power of p
inline double cMath::Power(double x, int p)
{
double x1 = x;
for (int i = 1; i < p; i++)
x1 *= x;
return x1;
}

Firstly, this function is wrong. Even if you restrict yourself to whole,
non-negative numbers, which isn't typically given, at least make sure that
Power(3.14, 0)==1.
Secondly, there is no reason to pass the "this" pointer to this function,
i.e. call this in the context of an object, since it doesn't use the state
of that object. You could make it a class-static function, you could also
make it a function at namespace scope (which includes global scope).
Lastly, don't bother fixing this, because the functionality you need is
provided by the standard library, the function is called pow(). :^)
Uli
--
C++ FAQ:http://parashift.com/c++-faq-lite
Sator Laser GmbH
Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932

Dear Ulrich,
How would you re-write my stuff if you were I? Could you re-write some
of my programs as an leading example?
Thanks
Jack

[Alex Blekhman]

Post by Luk Jack
How would you re-write my stuff if you were I? Could you re-write some
of my programs as an leading example?

Well, as Ulrich already said, there is standard `pow' function. So, you
don't need to have `cMath::Power' method at all. Just call "pow(3, 2)"
where you previously called "m->Power(3, 2)".

Alex

[Victor Bazarov]

Post by Ulrich Eckhardt

Post by Ulrich Eckhardt

Post by Luk Jack

Post by Ulrich Eckhardt
1. Wrapping something into a class that only contains functions and no
actual state (which is what your cMath class looks like, though I'm not
sure) is wrong. If you need, use a namespace instead. I guess you have
this habit and probably a few others from Java or similar languages that
force you to use classes for everything.

[...]

Post by Luk Jack
Actually, I've got an attribute in my cMath class, so isn't it
stateless, is it? not too sure :)

Yes, that would be a valid reason to put it into a class.
Uli
--
C++ FAQ:http://parashift.com/c++-faq-lite
Sator Laser GmbH
Geschäftsführer: Thorsten Föcking, Amtsgericht Hamburg HR B62 932

class cMath
{
cMath() { }
~cMath() { }
double Integrate(const Formula& f);
void Derive(const Formula& f);
Terms *Solve_Term(const Terms& t);
double Power(double x, int p);
// Formula m_vNomralFunctions; // Normal Functions for generating
approx function
const Formula& getFormula() { return m_vFormula; }
Formula m_vFormula;
};
inline void cMath::Derive(const Formula& f)
{
Terms *t1 = new Terms();
for (unsigned int i = 0; i< f.size(); i++)
{
t1 = Solve_Term(f[i]);

If this happens, the previous value of 't1' is lost. Since you allocate
't1' here, you lose *that* memory if you even enter the body of the
loop. And since you allocate something in 'Solve_Term', you are going
to lose *that* memory if your body executes more than once.

Consider switching to pure passing-by-value. Just stop using 'new' and
'delete' altogether.

Post by Ulrich Eckhardt
m_vFormula.push_back(*t1);
}
delete t1;
}
inline Terms *cMath::Solve_Term(const Terms& t)
{
Terms *t1 = new Terms();
t1->coefficient = t.coefficient;
t1->degree = t.degree-1;
t1->coefficient = t1->coefficient*t.degree;
if (t1->degree< 0)
t1->coefficient = 0;
//delete t1;
return t1;
}

V