5

Here you can see copy assignment operator implementation with self assignment check:

String & operator=(const String & s)
{
    if (this != &s)
    {
        String(s).swap(*this); //Copy-constructor and non-throwing swap
    }

    // Old resources are released with the destruction of the temporary above
    return *this;
}

This is good for self assignment, but bad for performance:

  1. as each time it check as if statement (I don't know how much will be it optimal, considering the branch prediction)
  2. Also we lose here copy elision for rvalue arguments

So I still don't understand if I would implement std::vector's operator= how I would implement it.

Narek
  • 38,779
  • 79
  • 233
  • 389
  • if you dont implement the self check than in the case of an assignment example: `A = A;` the value of object `A` will be deleted. As far as I know most implementation of assignment operator check for self assignment – HazirBot May 09 '16 at 20:53
  • 1
    Why will be deleted? You can implemented it in such a way that it is still safe but you don't check self assignment. – Narek May 09 '16 at 20:54
  • lets say `A` holds values on the heap. first step will be to delete these values, and than attach the values of rvalue to it. but since those values are the same you've already deleted them. – HazirBot May 09 '16 at 20:56
  • 1
    @GiladMitrani, you do not seem to understand copy and swap idiom. – SergeyA May 09 '16 at 20:57
  • @SergeyA I stand corrected by your answer. – HazirBot May 09 '16 at 20:58
  • 3
    If you care a lot about performance, don't do copy-and-swap. – T.C. May 09 '16 at 21:01
  • When I want to write `operator=` for `std::vector` I don't know what I care first. This is a general purpose code. – Narek May 09 '16 at 21:10
  • @Narek: Use copy-and-swap to get things up and working, but you should try to write a more efficient assignment operator as a goal. If someone needs strong exception safety, they can always make a copy and swap themselves. – GManNickG May 09 '16 at 21:11
  • You have `String` in your class but you say you're implementing `std::vector`? Well if you're implementing `std::vector` why are you using copy-and-swap in the first place? – uh oh somebody needs a pupper May 09 '16 at 21:45
  • 2
    Sure self assignment can happen and in that one case the test for self assignment will save you for an extra copy. **BUT** self assignment is very rare and as a result you are damaging the **much more common** case with branching. Of course your use case may be special so time it. – Martin York May 09 '16 at 22:17

2 Answers2

5

Yes, this code is superflous. And it is true that it is causing extra unncessary branch. With proper swap and move semantics, following should be much more performant:

String& String::operator=(String s) { // note passing by value!

    std::swap(s, *this); // expected to juggle couple of pointers, will do nothing for self-assingment
    return *this;
}

Also note, it is more benefical to accept the argument by value.

SergeyA
  • 61,605
  • 5
  • 78
  • 137
  • 1
    Argument by value is already a copy. For self-assignment you have already made a bad thing. Consider that vector is too large. – Narek May 09 '16 at 20:59
  • 1
    1) there's no reason to have `tmp` if you are taking by value. 2) `std::swap` will likely result in infinite recursion. – T.C. May 09 '16 at 20:59
  • @T.C., agree with tmp. An artifact of previous version, will remove. But why endless recursions with swap? It is expected to be specialized for `String` and juggle pointers. – SergeyA May 09 '16 at 21:01
  • @Narek, choose your pick. You either copy immediately and avoid branching, or you do self-check and copy than. – SergeyA May 09 '16 at 21:02
  • @Narek: Implement assignment as copy-and-swap is a "works every time" thing, at the cost of not being optimal for the task at hand (say, not duplicating a huge container). If you want to defer the copy, you need two overloads for assignment - one for lvalues and one for rvalues - so that when the time comes to copy you can properly move the ravlue. If you copy in the argument, this happens automatically for you. – GManNickG May 09 '16 at 21:02
  • Why passing by value is better? – Zereges May 09 '16 at 21:02
  • 3
    @SergeyA: In general, you should avoid explicitly calling `std::swap`. Use `using std::swap; swap(a, b)` to allow swap overloads to be found during ADL, or `boost::swap` which does this for you, or `std::iter_swap(&a, &b)` which also does this. Custom swap functions should be defined as a friend function in the same namespace as the class for this reason. – GManNickG May 09 '16 at 21:03
  • @GManNickG, it depends on habits, I'd say. Since you are allowed to put specializations into namespace `std`, it is a valid option. Obviously, there is more than one way to skin the cat. – SergeyA May 09 '16 at 21:05
  • 3
    @Zereges, my point is that actual self-assingment does not happen often (at least in my code). So copy elision for temoraries is very important - and this code allows for that. – SergeyA May 09 '16 at 21:07
  • If you actually have a specialization, that's OK. – T.C. May 09 '16 at 21:08
  • @T.C., well, with CaS idiom you are supposed to, aren't you? – SergeyA May 09 '16 at 21:09
  • @SergeyA: There is more than one way, but there may also be a best way. :) See http://stackoverflow.com/questions/11562/how-to-overload-stdswap for more discussion. Modern consensus is to do it so that ADL finds it. – GManNickG May 09 '16 at 21:09
  • @GManNickG, well Dave is certainly a knowlegeable person and everything, but while he claims it to be the *best*, he doesn't actually provide a comparison and explanation why it is better than specialization (unless I missed it somewhere in the comments). Can you provide condensed reasoning? – SergeyA May 09 '16 at 21:12
  • 2
    @SergeyA: See http://stackoverflow.com/a/8439357/87234in particular. tl;dr: ADL-found swap always works (as long as the swapper always allows ADL swap to be found, which the standard requires for its own implementations), std::swap specialization does not always work. – GManNickG May 09 '16 at 21:14
  • @GManNickG, see it now. Thanks! I will reconsider my habits here! – SergeyA May 09 '16 at 21:16
1

as each time it check as if statement (I don't know how much will be it optimal, considering the branch prediction)

I think you've got yourself in a premature optimization circle here.

Check for self assignment -> self-assignment is unnecessary if you wrote the code properly -> why not write swap explicitly if you mean it? -> we're back to square one

Realistically, I would just implement Allocator and not worry about it.

Also we lose here copy elision for rvalue arguments

I don't think so.

#include <iostream>

#define loud(x) std::cout << x << "\n";

struct foo
{
    foo() { loud("default") }
    ~foo() { loud("destruct") }

    foo(const foo&) { loud("copy") }
    foo(foo&&) { loud("move") }

    foo & operator=(const foo & s)
    {
        if (this != &s)
        {
            loud("copy assign")
        }

        return *this;
    }
};

int main()
{
    foo f;
    foo g;
    g = f;
}

Outputs:

default
default
copy assign
destruct
destruct

This is with -fno-elide-constructors.

You claim the branch may be a problem, but the assembly output for -O2 shows me that GCC doesn't even emit code for the operator= and just outputs the "copy assign" string directly. Yes, I realized I have a simplified example, but it really is the wrong end of the pile to start from.

uh oh somebody needs a pupper
  • 3,348
  • 12
  • 27