segmentation fault in overloading operator =

问题:

I just got a seg fault in overloading the assignment operator for a class FeatureRandomCounts, which has _rects as its pointer member pointing to an array of FeatureCount and size rhs._dim, and whose other date members are non-pointers:

FeatureRandomCounts &  FeatureRandomCounts::operator=(const FeatureRandomCounts &rhs)  
{  
  if (_rects) delete [] _rects;  

  *this = rhs;  // segment fault

  _rects = new FeatureCount [rhs._dim];  
  for (int i = 0; i < rhs._dim; i++)  
  {  
    _rects[i]=rhs._rects[i];  
  }  

  return *this;    
}

Does someone have some clue? Thanks and regards!

回答1:

As mentioned, you have infinite recursion; however, to add to that, here's a foolproof way to implement op=:

struct T {
  T(T const& other);
  T& operator=(T copy) {
    swap(*this, copy);
    return *this;
  }
  friend void swap(T& a, T& b);
};

Write a correct copy ctor and swap, and exception safety and all edge cases are handled for you!

The copy parameter is passed by value and then changed. Any resources which the current instance must destroy are handled when copy is destroyed. This follows current recommendations and handles self-assignment cleanly.

---

#include <algorithm>
#include <iostream>

struct ConcreteExample {
  int* p;
  std::string s;

  ConcreteExample(int n, char const* s) : p(new int(n)), s(s) {}
  ConcreteExample(ConcreteExample const& other)
  : p(new int(*other.p)), s(other.s) {}
  ~ConcreteExample() { delete p; }

  ConcreteExample& operator=(ConcreteExample copy) {
    swap(*this, copy);
    return *this;
  }

  friend void swap(ConcreteExample& a, ConcreteExample& b) {
    using std::swap;
    //using boost::swap; // if available
    swap(a.p, b.p); // uses ADL (when p has a different type), the whole reason
    swap(a.s, b.s); // this 'method' is not really a member (so it can be used
                    // the same way)
  }
};

int main() {
  ConcreteExample a (3, "a"), b (5, "b");
  std::cout << a.s << *a.p << ' ' << b.s << *b.p << '\n';
  a = b;
  std::cout << a.s << *a.p << ' ' << b.s << *b.p << '\n';
  return 0;
}

Notice it works with either manually managed members (p) or RAII/SBRM-style members (s).

回答2:

*this = rhs;

calls operator=(), which is the function you are writing. Cue infinite recursion, stack overflow, crash.

Also, if you used a std::vector rather than a C-style array, you probably would not need to implement operator=() at all.

回答3:

 *this = rhs;  // segment fault

This is definitively not the way to do it. You call = recursively, not calling the built in assignment operator. Assign variables one by one. Don't be lazy.

回答4:

The following line:

  *this = rhs;  // segment fault

will recursively call your operator=() function resulting in a stack overflow.

You should probably replace it with straight-forward assignments of the various member fields.

As Neil said, using something like std::vector<> will remove much of the responsibility away from your code. If for whatever reason you can't or don't want to use std::vector<>, you might also want to consider using the 'swap idiom' for your assignment operator. This will make the function exception safe (if the allocation of the memory for FeatureCount array fails and throws an exception, the original object that's being assigned to will be left unchanged). Something like the following:

void FeatureRandomCounts::swap( FeatureRandomCounts& other)
{
    FeatureCount* tmp_rects = other._rects;
    int tmp_dim             = other._dim;    // or whatever type _dim is

    // similarly for other members of FeatureRandomCounts...

    // now copy the other contents to 
    this->_rects = other._rects;
    this->_dim   = other._dim;

    // assign other members of rhs to lhs

    other._rects = tmp_rects;
    other._dim   = tmp_dim;

    // etc.

    return;
}

Now your assignment can look like:

FeatureRandomCounts &  FeatureRandomCounts::operator=(const FeatureRandomCounts &rhs)  
{  
    FeatureRandomCounts tmp( rhs);  // make a copy

    tmp.swap( *this);               // swap the contents of the copy and *this

    return *this;
                                    // the contents of tmp (which has the old 
                                    //  stuff that was in *this) gets destructed
}

Note that you need a proper copy constructor for this to work, but given the Big 3 rule you already need a proper copy ctor.