Class pointer and vector problem

[po0ky]

As the title shows, I find it hard to come up for a good description of the problem I encountered.

Please can some of you guru's help me?

#include <vector>

class sD
{
private:
   float* _d;
public:
    sD(){};
    ~sD(){ delete _d; }
   sD(const sD & D):_d(new float((*D._d))){}
   sD(float d){ _d = new float(d); }
   operator float(){ return *_d;
};

class tst{
private:
   std::vector<sD> vec;
public:
   tst(){};
   void insert(float sD){ vec.insert(vec.begin(), sD); }
   float operator[](std::vector<sD>::size_type i){ return vec[i]; }
};

int main(){
    tst t;
    t.insert(10);
    t.insert(20);
    float tmp1 = t[1];// tmp1 holds 10
    float tmp2 = t[0];// tmp2 holds ?
}

This is a distilled segment of the code. The actual code is much bigger.

I would expect tmp2 to be 20.
If I replace "vec.insert(vec.begin(), sD);" with "vec.push_back(sD);" than as expected; tmp2 holds 10 and tmp1 holds 20.

Why doesn’t the tmp2 of my code hold 20? How can I remedy this behaviour?

[Philip Nicoletti]

I get the expected output on my system (and it also crashes).
You should add an assignment operator to class sD

sD & operator = (const sD & rhs)
{
   *_d = *rhs._d;
   return *this; 
}

[Paul McKenzie]

As the title shows, I find it hard to come up for a good description of the problem I encountered.

The issue is that your class sD is not safely-copyable or assignable, and copying/assinging is exactly what vector does to insert items into itself.

You don't need vector to show you that you have a problem:

class sD
{
private:
   float* _d;
public:
    sD(){};
    ~sD(){ delete _d; }
   sD(const sD & D):_d(new float((*D._d))){}
   sD(float d){ _d = new float(d); }
   operator float(){ return *_d;
};

int main()
{
   sD s1(10);
   sD s2;
   s2 = s1;
}

That simple code above shows you have a memory leak and a double deletion error. There is no usage of vector in that code. All I'm doing is a simple assignment of your sD objects.

Your class lacks a properly coded assignment operator.

This is a distilled segment of the code. The actual code is much bigger.

I would hate to be the programmer maintaining this code that does the new/delete gymnastics all over the place. Instead, why exactly are you dynamically allocating a float? What's the reason for introducing new/delete here?

I would expect tmp2 to be 20.

What would you expect from the small three-line program I posted? No user-defined assignment operator, and all bets are off.

Regards,

Paul McKenzie

[Paul McKenzie]

I get the expected output on my system (and it also crashes).
You should add an assignment operator to class sD

Yes, but the code is not enough -- there still is a memory leak if the original memory is not deleted first.

Regards,

Paul McKenzie

[po0ky]

Thank you both for the fast reply...
Thank you for your answer Philip Nicoletti, it works.

@Paul McKenzie
I know I need to check for self-assignment....

The reason for the new/delete gymnastics is that I want my object to behave like a built in object. I just got the new/delete gymnastics in this class and only in this class, so I'm not wasting memory with classes holding and maintaining copies of the same values.

I can't post the whole code and expect you to read it all... I'm pretty sure I can't do without new/delete.

Anyway Paul, thanks for the heads-up about those memory leaks. Maybe you can save me the search and point them out?

[Paul McKenzie]

How can I remedy this behaviour?

The right solution is to strive not to code this way.

A C++ programmer should attempt to use dynamic allocation only when necessary. Just doing dynamic allocation just for the sake of doing it, or because the coder is not experienced enough to know better aren't reasons to be coding in the manner that you've presented.

You have smart pointer classes, containers, other RAII type objects, etc. that remove the need to micromanage allocations and deallocations.

Regards,

Paul McKenzie

[Paul McKenzie]

Thank you for the fast reply...
And it works too .

The code that Philip gave you is not enough -- take my example main() program, and you will see that you corrupt memory when main() exits. It needs to be changed to add the deletion of the original pointer, which Philip's code does not show.

Guess its how vector::insert works.

It isn't just how vector works. You saw the simple program I posted, and it still has a bug.

Any object that isn't safely copyable used in any program is susceptible to causing bugs. If a class cannot be copied, then the proper way to turn off the copying is to declare a private copy constructor and assignment operator, and leave them unimplemented.

Otherwise, anyone can take your class and do things with it you don't expect, such as I did with my post, and again, you saw no usage of vector there.

But, dont I need to check for selfassignment there?

In the simple code, yes.

Regards,

Paul McKenzie

[po0ky]

I bet one of the things wrong with my code is that I dont initialize my variables?

[po0ky]

And i got to check if the pointer is pointing to something before deleting it?

[VictorN]

And i got to check if the pointer is pointing to something before deleting it?

No, if you initiated that pointer as NULL from the very begin.

[po0ky]

Thank you VictorN.

@McKenzie
I've given it some thought but I believe you're wrong. The fact is... you don't know all the details about how I'm implementing the class. The sD class isn’t supposed to be used directly. So luckily, I don’t feel obligated to provide code for all imaginable things one might want to do with it.

I've tried to explain it all and tried to write a reply to convince you about how wonderful my program is going to be... but after writing an hour long I came to my senses and decided that you probable already made your assumptions and writing an extremely long reply is just a waste of time.

[Paul McKenzie]

Thank you VictorN.

@McKenzie
I've given it some thought but I believe you're wrong. The fact is... you don't know all the details about how I'm implementing the class.

It doesn't matter -- one detail I do know is that your class is not safely copyable as you've written. I proven that with a 2 line main() program.

As much as you may believe you can The sD class isn’t supposed to be used directly.

It doesn't matter who uses it -- the C++ language doesn't care who uses it. As long as it shows up somewhere in the code, whether it is your code or other code, you should not leave classes like that exposed to copying bugs.

So luckily, I don’t feel obligated to provide code for all imaginable things one might want to do with it.

I didn't need to write a huge monolith of code to show you where your class is incorrectly coded. Copying and assigning are essential tasks to a C++ program, so essential that sometimes, the compiler makes copies of your object without you being aware of it. If the compiler or yourself copies objects, your object is in deep trouble since it cannot be counted on to reliably work in the simplest of scenarios. If a tiny two line program can cause your objects to go haywire, can you imagine a program that is thousands of lines long, where there is no way a programmer can eyeball every single place that this object is used and what the compiler may be doing with it?

To enforce that an object cannot be copied:

class sD
{
private:
   float* _d;
   sD(const &sD);
   sD& operator = (const &sD);
public:
    sD(){};
    ~sD(){ delete _d; }
};

This would force the compiler or linker to now emit an error if in any way, shape or form, the sD class is being used to make copies, either by you, some other programmer, or by the compiler itself. The reason is that copying and assigning has been turned off by moving those functions to the private section, and leaving them unimplemented. This would also mean that you can't place these in vectors, pass or return them by value, or do basic "=" assignments. If you want all of those things I mentioned, then you need to guarantee that copying that sD object is safe to do, and that can only be done by properly coding it to be safely copyable.

I've tried to explain it all and tried to write a reply to convince you about how wonderful my program is going to be... but after writing an hour long I came to my senses and decided that you probable already made your assumptions and writing an extremely long reply is just a waste of time.

The assumptions I've made is based on what you've posted and what you're trying to do. You are using std::vector, and vector makes copies -- doing that requires your type that you're placing in the vector to be safely copyable and assignable. The original code you posted is not safely copyable or assignable.

As to your program, all C++ programs should be built on basic, sound principles. One of those for C++ is usage of RAII, smart pointers, containers and algorithms, etc. and only use dynamic memory allocation if there is a sound, valid reason to do so. If the code you posted followed those principles, then you wouldn't have the issue you started the thread with. I personally have had to maintain and debug a class that looked similar to yours. There were so many memory allocation/deallocation bugs, I got rid of the pointers, new/delete used within the class, and replaced it with container objects. The problems went away immediately after those changes were made.

Also, as far as new/delete goes, any experienced C++ programmer would say the same thing to you as I've stated -- you don't use new/delete just because it's available. Dynamically allocating memory for any reason should be justified.

Regards,

Paul McKenzie