assignment operator for an static array inside a class

[acosgaya]

Hi I have the following code (shown below) which is a class brt_node with 3 members, one of them being an static array called "buffer", the elements of this buffer are pairs (int, CDataType), the CDataType is also shown.

I have created my own copy constructors and assignment operators, so that when I do something like the in following lines, I don't have to copy all the buffer, but only the part I am interested in, and given by the function get_buffer_size()

brt_node x;
//... do something with x
brt_node n = x; // copy constructor
brt_node y;
y = n; // assignment operator

I know that executing the lines above will work (without my own copy constructor and assigment operator), c++ takes care of it, but is very slow (I don't need to have the whole buffer copied);
The copy constructor and assigment operator I have defined, do not seem to work properly (segmentation fault sometimes);

Btw, the array has to be static, don't want a dynamic array in this specific case.

I would appreciate some help fixing it. Thanks in advance.

class CDataType{
public:
  // constructors:
  CDataType():
    vertex(0), 
    number(0){}
    
  CDataType(const unsigned int &_v, const cases &_status = unexplored, const unsigned int &_number = 0):
    vertex(_v), 
    number(_number){}
  
  const CDataType &operator=(const CDataType &e) {
    if ( &e != this){
      this->vertex = e.vertex;
      this->number = e.number;
      return (*this);
    }
  }  
  
  // -- attributes
  unsigned int vertex;
  unsigned int number;
};


// -- type of each record stored in the brt_node's buffer
typedef std::pair<unsigned int, CDataType> value_type;  


class brt_node {
  
public:
  
  // constructors
  brt_node():
    left(0), two_children(false) 
    { buffer[0] = value_type(0, CDataType(0) ); }; 
  
  brt_node(const value_type &_key_pair):
    left(0), two_children(false) 
    { buffer[0] = _key_pair; }
  
  brt_node(const unsigned &_key):
    left(0), two_children(false) 
    { buffer[0] = value_type(_key, CDataType(0) ); }
  
  // copy constructor
  brt_node(const brt_node &_n):
    left(_n.get_leftChildPos()),
    two_children(_n.has_2_children())
    { std::copy(_n.buffer, _n.buffer+_n.get_buffer_size()+1,buffer); }
  
  // functions

  KeyType get_key() const { return buffer[0].first; }  
  void    set_key(const KeyType &_key) { buffer[0] = value_type(_key, CDataType(0)); }
  
  KeyType get_buffer_size() const { return buffer[0].second.vertex; }  
  void    set_buffer_size(const unsigned int &size) {  buffer[0].second.vertex = size; }
  

  bool has_2_children() const { return two_children; }    
  void set_has_2_children(const bool &_two_children) { two_children = _two_children; }
  
  unsigned int get_leftChildPos() const { return left; }  
  void         set_leftChildPos(const unsigned &_index) { left = _index; }
    
  
  // -- object assignment operator
  const brt_node &operator=(const brt_node &n) {
    if ( &n != this){
      left         = n.get_leftChildPos();
      two_children = n.has_2_children();
      std::copy(n.buffer, n.buffer+n.get_buffer_size()+1, buffer);
      return (*this);
    }
  }  
  
  value_type buffer[BUFFER_SIZE];

private:
  
  unsigned int left;      
  bool two_children;      
};

[Paul McKenzie]

Hi I have the following code (shown below) which is a class brt_node with 3 members, one of them being an static array called "buffer", the elements of this buffer are pairs (int, CDataType), the CDataType is also shown.

Your CDataType does not need a user-defined copy constructor or assignment operator. Both members are unsigned int's, and they are copyable using the compiler default version of these functions.

Do not introduce copy and assignment operators for no reason. What ends up happening many times is that you now have to maintain any new member variable that's added to the CDataType class, making sure that you copy them and assign them. By using the compiler default, you let the compiler handle these things, not user code.

The only times when you need to define a user-defined copy and assignment are one or more of the following:

1) Your class contains members that are pointers to dynamically allocated memory.

2) Your class contains members that are references.

3) Your class is derived from another class that has turned off copying and assignment.

4) Your class contains members that represent resources that shouldn't be blindly copied (i.e. file handles, printer handles, etc.).

5) Your class is implementing a reference-counting scheme to track copies (similar to various copy-on-write string classes).

Otherwise, the compiler default is perfectly fine and should be used.

Edit:
Added fifth item:

Regards,

Paul McKenzie

[Paul McKenzie]

In addition, I don't see a reason for your brtnode class for having a user-defined copy constructor and assignment operator. None of what you've shown needs any of that code you've written.

I have created my own copy constructors and assignment operators, so that when I do something like the in following lines, I don't have to copy all the buffer,

Why? When a copy is made, it's supposed to represent an exact copy, not a mutation of that copy. Writing code to produce "fake" copies will lead to hard to find bugs and a whole host of maintenance to make it work right.

The only exception is if you're creating a reference counting scheme (That should have been the fifth item in my last post, which I forgot to mention). Otherwise, you're just asking for trouble.

If you removed all of that code to copy and assignment, does your program work? It should, as all you have are int's, bools, and maps (I see no pointers).

Regards,

Paul McKenzie

[acosgaya]

thanks for the reply, yeah it works without the constructor and operator, the problem comes when I defined my own.

In this specific case when letting the compiler takes care of it is very slow (copies the whole buffer, even if I don't need all).

If I declare a variable, say

brt_node x, n;

//... do something with x

n = x; // I don't need all the buffer copied, just some elements

if I do something like

std::copy(x, x+x.get_buffer_size(), n); 
// x.get_buffer_size() might be less than the original buffer_size

is faster than the direct assignment n = x (of course I also have to copy the other two variables)

I want that behavior in order to speed it up, because I will be doing this assignment a lot;

thanks

[Paul McKenzie]

thanks for the reply, yeah it works without the constructor and operator,

OK. So let's stay with this.

the problem comes when I defined my own.

In this specific case when letting the compiler takes care of it is very slow (copies the whole buffer, even if I don't need all).

That is very hard to believe that copying an array is so slow. Arrays being copied are usually just a memset() internally. I would not be surprised if your code is even slower than what a compiler would do.

Add to that, why did you code a user-defined copy ctor and assignment operator for CDataType? There was absolutely no reason to do so in that case, as all it has are two int members.

You need to show proof of this slowness, as others will more than likely disprove your claims. Also, you need to run profiling tests to make sure you are not wasting your time optimizing something that makes no difference.

You also need to specify which compiler, version, etc. and whether you are testing an optimized version, or a non-optimized (i.e. "debug" ) version. You never time debug versions, which is why you need to provide more information.

Bottom line is that there is no need for all of this code, unless you have evidence of what you're doing. In the industry, your code would be rejected without evidence of having to write it, especially if your code works without all of this extra baggage and maintenance.

Regards,

Paul McKenzie

[Paul McKenzie]

I don't need all the buffer copied, just some elements

Then IMO, this is an abuse as to what a copy is supposed to be. If you're not copying all of the elements, or at the very least, effectively making a copy, then it isn't a copy.

In other words, after your copy, this relationship must be true:

SomeObject a;
SomeObiect b = a;
SomeObject c;
c = a;
//...
if ( a == b ) 
{
    if ( b == c )
    {
        if ( a == c )
        {
             // copy is a true copy
       }
    }
}

The if()'s are theoretical in nature. If the objects are not equivalent to each other, then they are not copies. For the rest of the program, whether I specify using an "a", "b", or "c" object should make no difference. If the program behaves any differently if I specify an a, b, or c, then an a is not b is not c, and your copies are not copies.

If anything, another function should be provided, and not use the assignment operators and copy constructor for this..

Regards,

Paul McKenzie

[acosgaya]

you are right, it isn't really an exact copy, ... I should wrap the desired behavior in function that does copy only the part I want. I guess I wanted a shortcut, but it isn't gonna work that way.

thanks