Copy constructor invokes infinite loop although having call by reference

Question

The code invoking the loop:

Foo temp = Foo(token.substr(0,i));
this->leftExpression = temp;

Having Foo declared as:

class Foo{
    private:
        std::string expr;
        std::vector <Bar*> tokens;
        std::vector <std::string> stringtoken;

And the CCTOR invoking the loop:

Foo::Foo(const Foo& a_expr){
    this->expr = a_expr.expr;
    this->tokens = a_expr.tokens;
    this->stringtoken = a_expr.stringtoken;
}

What is invoking this loop?

EDIT:

Assignment operator:

Foo& Foo::operator=(const Foo& a_expr){
    Foo temp(a_expr);
    std::swap(*this, temp);
    return (*this);
}

I can only tell you one thing: It's not caused by the code you are showing to us. — Daniel Frey, Apr 21 '14 at 15:02
There's nothing obviously wrong with the code you're posted (although the copy is unnecessary; why not just write `Foo temp(...);`?). Could you post a complete test case to demonstrate the error? — Mike Seymour, Apr 21 '14 at 15:03
Show us the constructor invoked by `Foo(token.substr(0,i));` because `token.substr(0,i)` definitely isn't `const Foo&`. Also, if `this->leftExpression = temp;` is part of the problem, show us the implementation of `Foo::operator=(const Foo&)`. — Mike DeSimone, Apr 21 '14 at 15:03
Is the problem caused by `this->leftExpression = temp;`? That's calling the copy-assignment operator, not the copy constructor. — Mike Seymour, Apr 21 '14 at 15:04
as [Lazlo Papp remarks](http://stackoverflow.com/questions/23199928/copy-constructor-invokes-infinite-loop-although-having-call-by-reference#comment35486110_23199928), please post a short, self-contained example that we can compile and run. — Cheers and hth. - Alf, Apr 21 '14 at 15:06

score 5 · Accepted Answer · answered Apr 21 '14 at 15:08

5

The problem is here:

std::swap(*this, temp);

The default implementation of std::swap uses assignment, hence the infinite recursion when calling it from your assignment operator.

If you really need to write your own assignment operator, then write your own swap, for example:

void Foo::swap(Foo & other) {
    using std::swap;
    swap(expr, other.expr);
    // and for all other members
}

Foo& Foo::operator=(Foo temp){
    this->swap(temp);
    return (*this);
}

In this case, it looks like all the members are correctly copyable (although you might need to be careful of the dumb pointers in tokens). If that is the case, then there's no need to write your own destructor, copy constructor or copy-assignment operator at all - just let the implicit ones do the right thing.

answered Apr 21 '14 at 15:08

Mike Seymour

249,747
28
448
644

Does this mean that the way I implemented the `operator=` will never work under any conditions? – Alexandru Barbarosie Apr 21 '14 at 15:12
@AlexandruBarbarosie See the link in my answer about the copy-and-swap-idiom which explains how to do it correctly. – Daniel Frey Apr 21 '14 at 15:12
@AlexandruBarbarosie: It could work if you specialised `std::swap` for your class. That would be a rather convoluted way to do it though. You can read more about the copy and swap idiom [here](http://stackoverflow.com/questions/3279543). – Mike Seymour Apr 21 '14 at 15:13
@DanielFrey apparently I misunderstood it, because that is was what I read before coming up with this implementation. – Alexandru Barbarosie Apr 21 '14 at 15:14
@AlexandruBarbarosie I see. Well, read it again and also use Mike's example implementation above until you understand the difference then. :) Mike: +1 – Daniel Frey Apr 21 '14 at 15:15
1

@AlexandruBarbarosie: The example there implements `swap` as a non-member (friend) function, then calls `swap(*this, other)` - **not** `std::swap`. That's very similar to my example of a member function - and in some ways better, since it overloads `std::swap`. – Mike Seymour Apr 21 '14 at 15:19

score 1 · Answer 2 · edited May 23 '17 at 11:44

1

Aaaand there's your problem: The assignment operator creates a copy and calls std::swap, which in turn calls the assignment operator.

You could either implement the assignment operator without using std::swap, or you could implement swap without using the default implementation and instead use the "copy-and-swap"-idiom.

edited May 23 '17 at 11:44

Community

1
1

answered Apr 21 '14 at 15:07

Daniel Frey

55,810
13
122
180

Copy constructor invokes infinite loop although having call by reference

2 Answers2