assigning classes by value

This is a discussion on assigning classes by value within the C++ Programming forums, part of the General Programming Boards category; ran into some trouble with this because the destructor for the LHS was called after the assignment, so it assigned ...

  1. #1
    3735928559
    Join Date
    Mar 2008
    Location
    RTP
    Posts
    839

    assigning classes by value

    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?

  2. #2
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  3. #3
    3735928559
    Join Date
    Mar 2008
    Location
    RTP
    Posts
    839
    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;
    }
    Last edited by m37h0d; 12-15-2008 at 09:20 PM.

  4. #4
    3735928559
    Join Date
    Mar 2008
    Location
    RTP
    Posts
    839
    hmm, it appears it is not automatically converting the instance to a reference. changing the operator argument to a value type eliminates the error.

  5. #5
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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...
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  6. #6
    3735928559
    Join Date
    Mar 2008
    Location
    RTP
    Posts
    839
    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.

  7. #7
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    22,276
    Quote 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.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  8. #8
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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).
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  9. #9
    3735928559
    Join Date
    Mar 2008
    Location
    RTP
    Posts
    839
    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.

  10. #10
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    22,276
    Quote 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>.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. assigning to static map<>s in classes
    By drrngrvy in forum C++ Programming
    Replies: 6
    Last Post: 10-18-2005, 10:05 PM
  2. im extreamly new help
    By rigo305 in forum C++ Programming
    Replies: 27
    Last Post: 04-24-2004, 12:22 AM
  3. Exporting VC++ classes for use with VB
    By Helix in forum Windows Programming
    Replies: 2
    Last Post: 12-29-2003, 05:38 PM
  4. Prime Number Generator... Help !?!!
    By Halo in forum C++ Programming
    Replies: 9
    Last Post: 10-20-2003, 08:26 PM
  5. include question
    By Wanted420 in forum C++ Programming
    Replies: 8
    Last Post: 10-17-2003, 04:49 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21