3

Given the code

struct Foo{
  Foo(const Foo &other){
    i = other.i;
  };

  Foo &operator=(const Foo &other){
    if(this == &other){
      return (*this);
    }

    new (this) Foo(other);
    return (*this);
  };
  
  int i = 0;
};


struct Bar{
  Bar &operator=(const Bar &other){
    if(this == &other){
      return (*this);
    }

    this->i = other.i;
    return (*this);
  };

  Bar(const Bar &other){
    (*this) = other;
  };
  
  int i = 0;
}

Assuming everything in the class is copyable, is it a good practice to avoid some repeated codes(consider classes with a lot of members) in the two forms above(class Foo and Bar), any potential danger? If so, which one is better?

Theo Mars
  • 41
  • 4
  • Placement new to `this` in a copy constructor is never good practice. There may be a legit use case, but I'm unable to think of one. (However, I am but a bear of little brain.) – Eljay Dec 28 '21 at 12:50
  • 2
    Both are bad practice. One consequence of both approaches is that copying or assigning any classes derived from `Foo` or `Bar` will not work properly (or, at best, will be quite difficult to get working correctly). – Peter Dec 28 '21 at 13:01
  • 1
    Any particular reason for using placement new? A standard solution for avoiding this code repetition is the _copy-and-swap_ idiom. For instance, in this live demo, the generated machine code for both options is the very same: https://godbolt.org/z/8bn7zbhM4. So why placement new? – Daniel Langr Dec 28 '21 at 13:01
  • 1
    Not a good idea in general; change the member type from `int` to `std::string` and you've created a memory leak. `std::string` is copyable... – fabian Dec 28 '21 at 13:24
  • 1
    You need to manually call the destructor before doing placement new to correctly destroy any member variable and base classes. If the class contained a `std::string` member variable, for example, the placement new would overwrite the `std::string` leaking the memory it used to own. – Richard Critten Dec 28 '21 at 13:26
  • 2
    You run into issues if the copy constructor could throw with placement new. And "consider classes with a lot of members": Use the default copy assignment operator/copy constructor. If you need to do certain things while copying, your class shouldn't also have lots of members (break it into multiple classes and compose, apply the least responsibility principle) – Artyer Dec 28 '21 at 13:35
  • 1
    Placement new has a lot of gotchas and you shouldn't use it if it isn't necessary. Here `Bar` clearly demonstrates that it isn't necessary. In both solutions you have to, at some point, enumerate the members. There isn't really any obvious benefit to using `Foo`'s approach to the problem. – François Andrieux Dec 28 '21 at 14:23
  • None of these uses of `(*this)` need parentheses. – Davis Herring Dec 28 '21 at 21:13
  • @DanielLangr no particular reason, I'm relatively new to the language, and still learning, thanks for the solution you post! – Theo Mars Dec 29 '21 at 02:06
  • @FrançoisAndrieux Thanks for the comment. The reason for the codes is there's only one necessary enumeration of members. What's the common way? Also, Is Bar a common pratice? – Theo Mars Dec 29 '21 at 02:14
  • @Artyer I'm learning the language, the least responsibility principle, that's a new one to me, thank you! – Theo Mars Dec 29 '21 at 02:22
  • @TheoMars `Bar` also only enumerates members once. That solution is okay and is usually fine, but it doesn't handle more complex situations, such as when members are not default constructible. Another solution is to implement and use a member `swap` function – François Andrieux Dec 29 '21 at 14:05

0 Answers0