Printable View
ran into some trouble with this because the destructor for the LHS was called after the assignment, so it assigned properly and then proceeded to delete itself.
reading the rules for assignments in my compiler documentation, it connotes that copying classes by value does not conform with its rules for non-arithmetic assignment operators.
is this generally true? does one generally have to overload the assignment operator to copy 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:
i get " E2285 Could not find a match for 'secs2MsgItem<AnsiString>::operator =(secs2MsgItem<AnsiString>)'"Code://DATAID and CEID are Strings
secs2MsgItem<String>ID(DATAID);
report->addDatum(ID);
ID=secs2MsgItem<String>(CEID);
report->addDatum(ID);
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©)
{
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;
}
hmm, it appears it is not automatically converting the instance to a reference. changing the operator argument to a value type eliminates the error.
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.Code:// in header file
template<typename T> secs2MsgItem<T>& operator =(secs2MsgItem<T>&RHS)
{
delete[]message;
message = RHS.message;
RHS.message = 0;
return *this;
}
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...
message is a block of binary data that is of variable length and may contain null characters. it represents a SECS II message item.
the default constructor is initialized that way because of the SECS II standard. the first byte contains two pieces of information, the upper 6 bits contain the format code (which denotes the data type of the message item) and the lower 2 bits tell the number of bytes immediately following that denote the length of the item. zero is an illegal value for the number of length bytes, but zero is an allowed value in a length byte. i have other methods that dynamically add data onto the end of message; i just wanted to make sure that the item header is always initialized to something valid (a list with 0 sub-items, in this case).
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
also, my compiler didn't like references in the assignment operator argument. not sure what, if any, the implications are on safety of self-assignment there.
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.Quote:
Originally Posted by m37h0d
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.
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:Quote:
not sure what, if any, the implications are on safety of self-assignment there.
Code://do something only if it is not a selfassignment
if (this != &RHS)
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.Quote:
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
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).
the copying is not a problem now. my messages are only a few kb at most at the moment. i just wanted to give some thought to optimizing now while it was on my mind.
i went ahead and changed it so the assignment operators take const secs2MsgData/secs2MsgItem<T> and copy rhs.message
i desperately wanted to use strings (or AnsiStrings) to no avail. it always terminated the data at the first 0x00 byte :\
i've actually had these message classes for a while, and the whole secs II interface system built on top of them works very well. i just had to change my strategy a little bit recently. before, secs2MsgData didn't delete message, the class that received and transmitted it did. obviously, though, that ended up having some limitations i couldn't live with, so i have come to make the modifications i'm detailing here.
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>.Quote:
Originally Posted by m37h0d