Thread: assigning classes by value

If your class is "simple" (POD), then you can rely on the compiler copying it - as it supports a "copy byte by byte" copy mechanism. So integers, floating point and arrays are fine

If you have pointers in your class, then you need to perform a "deep copy" - meaning allocating memory and copying content into the allocated memory, as a byte by byte copy will just copy the original pointer value, which, as you say, may be deleted an instant later and thus "disappear from the surface of the planet" (or worse, get overwritten by some completely wrong data values when it's been allocated again - which can be tricky to debug).

--
Mats

the class in question doesn't actually require new allocation in the assignment, i actually just want it to reassign the pointer, but it's calling the destructor after the assignment, so it's deleting the newly assigned message from the asignee. (or at least i think that's what it's doing)

i'm having to trouble using the assignment operator. this class has specialized template descendants, but when i try to do the following:

Code:

//DATAID and CEID are Strings

                        secs2MsgItem<String>ID(DATAID);
                        report->addDatum(ID);

                        ID=secs2MsgItem<String>(CEID);
                        report->addDatum(ID);

i get " E2285 Could not find a match for 'secs2MsgItem<AnsiString>:perator =(secs2MsgItem<AnsiString>)'"
i even added a template assignment operator, but it's just not matching it in the base class. is it because the derived classes are specialized and this function isn't?

here's the c'tors, d'tor and the newly added assignment operator. anything bad sticking out at you?

Code:

// in header file

                template<typename T> secs2MsgItem<T>& operator =(secs2MsgItem<T>&RHS)
                {
                        delete[]message;
                        message = RHS.message;
                        RHS.message = 0;
                        return *this;
                }

//in implementation file

__fastcall secs2MsgData::secs2MsgData() :
        message(new char[2])
{
        message[0]=1;
        message[1]=0;
}

__fastcall secs2MsgData::secs2MsgData(char *_message) :
        message(_message)
{
        unsigned int l;
        if(formatCode())
        {
                l = length();
        }
        else
        {
                l = headerLength()+((secsList*)this)->secsList::dataLength();
        }
        char *c = new char[l];
        copychar(c,_message,l);
        message =c;
}

__fastcall secs2MsgData::secs2MsgData(const secs2MsgData&copy)
{
        unsigned int l=copy.length();
        message = new char[l];
        copychar(message,copy.getMsg(),l);
}

__fastcall secs2MsgData::~secs2MsgData()
{
        if(message)
        {
                delete[]message;
        }
}

secs2MsgData& secs2MsgData::operator =(secs2MsgData&RHS)
{
        delete[]message;
        message = RHS.message;
        RHS.message = 0;
        return *this;
}

Code:

// in header file

                template<typename T> secs2MsgItem<T>& operator =(secs2MsgItem<T>&RHS)
                {
                        delete[]message;
                        message = RHS.message;
                        RHS.message = 0;
                        return *this;
                }

This is somewhat bad (in the same sense as auto_ptr, so the class might be usable for some purposes if you are careful). For example, you can't store instances of this class in a STL container. Normally operator= should not modify the right-hand value.

It is also not safe against self-assignment.

If this is what you need, you might write an auto_array_ptr, similar to auto_ptr (with the difference that it uses delete[]) and then make the message member an auto_array_ptr<char>. I would disable copying and assigning for classes that have auto_ptr members, though.

Or, looking at your code, is message a string that is always exactly one character long? If so, why not use a single char (or plain array char[2]). It looks somewhat bizarre to see someone allocate one or two char's dynamically...

Originally Posted by m37h0d

these messages may be several MB, so i wanted to avoid superfluous copying. that's why i reassigned the message pointer of RHS instead of copying it to this/LHS

Give the class normal copy semantics instead, possibly using std::vector instead of manually managing memory, and then use a smart pointer (std::tr1::shared_ptr seems appropriate) if necessary.

For one thing you might use a std::string for it then (it supports embedded '\0's as long as it is not interacting with a C-style string), or you might write (or find) a RAII class that understands the format and can look after the memory itself.

not sure what, if any, the implications are on safety of self-assignment there.

The implication of self-assignment is that message and RHS.message will refer to the same thing, and hence this member will be set to 0 for both operands. You can check for self-assignment:

Code:

//do something only if it is not a selfassignment
if (this != &RHS)

these messages may be several MB, so i wanted to avoid superfluous copying. that's why i reassigned the message pointer of RHS instead of copying it to this/LHS

Then you end up with a class whose usage is limited. You must not pass it by value (as it zeros out the caller's object), and you shouldn't store it in STL containers, particularly not in a vector.

However, I would rather look for ways how to minimize needless copying, rather than cripple the copy operations. For example, hold (smart) pointers to this object - zero copying. Or see where the copying occurs and if it can be avoided (reserve memory before-hand, use a list instead of vector, allow two-step construction - setting the buffer after the object is created and inserted into a container etc).

But first you have to be sure that copying is a problem in the first place (get it working, then make it work faster).

Originally Posted by m37h0d

i desperately wanted to use strings (or AnsiStrings) to no avail. it always terminated the data at the first 0x00 byte :\

A std::string object can contain embedded null characters, but then using c_str() would not make sense. You would use data() and pass the size (otherwise there is no way to know when the string ends). If you need the data to be stored contiguously then use a std::vector<char>.