2

There are three way to use keyword 'new'. First is the normal way. Suppose Student is a class.

Student *pStu=new Student("Name",age);

Second way . Only ask for the memory space without calling the constructor.

Student *pArea=(Student*)operator new(sizeof(student));//

Third way is called 'placement new'. Only call the constructor to initialize the meomory space.

new (pArea)Student("Name",age);

So, I wrote some code below.

class Student
{
private:
    std::string _name;
    int _age;
public:
    Student(std::string name, int age):_name(name), _age(age)
    {
        std::cout<<"in constructor!"<<std::endl;
    }
    ~Student()
    {
        std::cout<<"in destructor!"<<std::endl;
    }
    Student & assign(const Student &stu)
    {
        if(this!=&stu)
        {
            //here! Is it a good way to implement the assignment?
            this->~Student();

            new (this)Student(stu._name,stu._age);

        }
        return *this;
    }
};

This code is ok for gcc. But I'm not sure if it would cause errors or it was dangerous to call destructor explicitly. Call you give me some suggestions?

Donglei
  • 275
  • 1
  • 10
  • 3
    Placement new is the third form. But the answer to your question is "No". The way to implement this assigment is to do nothing. It comes for free. – R. Martinho Fernandes Jul 05 '13 at 12:23
  • That's not what "placement new" is. – Kerrek SB Jul 05 '13 at 12:26
  • @R.MartinhoFernandes In his particular case, the assignment operator comes for free. In the general case, what the compiler gives you, however, may not be what you need. And even in this case, you might not want it to be inline. – James Kanze Jul 05 '13 at 12:53
  • @R.MartinhoFernandes and KerrekSB. yes, placement new is the third form. – Donglei Jul 05 '13 at 13:47

3 Answers3

5

The problem with a "replacement-assignment" is that it's not exception safe. Consider this simplified, generic approach:

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

        ~Foo();
        ::new (this) Foo(rhs);   // may throw!
    }

    // ...
};

Now if the copy constructor throws an exception, you're in trouble. You've already called your own destructor, so the inevitable next destructor call will cause undefined behaviour. You also cannot change the order of operations around, since you don't have any other memory.

I actually asked about this sort of "stepping on a landmine" behaviour in a question of mine.

Community
  • 1
  • 1
Kerrek SB
  • 464,522
  • 92
  • 875
  • 1,084
  • 2
    The implications if you derive are also interesting. – James Kanze Jul 05 '13 at 12:51
  • It's probably also worth pointing out that his initial code contained a test for self assignment. The necessity of such a test is often an indication that the assignment isn't thread safe. – James Kanze Jul 05 '13 at 12:56
  • @JamesKanze: I had just forgotten to add the test while I was briefly thinking about taking the RHS by value. Fixed. – Kerrek SB Jul 05 '13 at 12:58
  • @JamesKanze Did you mean to say exception-safe here? Worrying about thread-safety in assignment is rare, at least for the lhs object. – Sebastian Redl Jul 05 '13 at 13:16
  • @SebastianRedl Yes. I definitely mean exception safety. Thread safety is an entirely different issue (and may not be relevant if the code isn't multithreaded.) – James Kanze Jul 05 '13 at 13:26
  • @KerrekSB : Yes, it is not exception-safe. – Donglei Jul 05 '13 at 14:02
0

Your suggestion is a major anti-pattern: if the new terminates with an exception, you'll get undefined behavior, and if someone tries to derive from your class, all sorts of weird things may occur:

DerivedStudent a;
DerivedStudent b;
a = b;      //  Destructs a, and reconstructs it as a Student

and when a goes out of scope, it is the destructor of DerivedStudent which will be called.

As a general rule, if you have to test for self-assignment, your assignment operator is not exception safe.

The goal of this idiom, of course, is to avoid code duplication between the copy constructor and the assignment operator, and to ensure that they have the same semantics. Generally, the best way to do this is to use the swap idiom, or something similar:

Student&
Student::operator=( Student const& other )
{
    Student tmp( other );
    swap( tmp );
    return *this;
}

void
Student::swap( Student& other )
{
    myName.swap( other.myName );
    std::swap( myAge, other.myAge );
}

(One final, unrelated point. In practice, you're going to run into conflicts with names which begin with an underscore. In general, it's best to avoid leading or trailing underscore.)

James Kanze
  • 150,581
  • 18
  • 184
  • 329
  • And not thread-safe. My mistake. I did not speak clearly. What I want to ask is about the 'placement-new' . To be honest, I did not realized this problem. Thank you for pointing out. – Donglei Jul 05 '13 at 14:25
  • @JamesKanze : "run into conflicts with names which begin with an underscore". Why? – Donglei Jul 05 '13 at 14:29
  • @KerrekSB Yup. I've seem to have threads on my mind today. This time, at least, I can fix it. – James Kanze Jul 05 '13 at 16:32
  • @Donglei Because for historical reasons, there are a number of system headers which define macros which begin with an underscore. Theoretically, they should only do so if it is followed by a capital letter, but practice doesn't always agree. – James Kanze Jul 05 '13 at 16:34
0

But I'm not sure if it would cause errors or it was dangerous to call destructor explicitly. Call you give me some suggestions?

The code as you wrote it has three major drawbacks:

  • it is difficult to read

  • it is optimized for the uncommon case (always performs the self assignment check although you rarely perform self assignment in practice)

  • it is not exception safe

Consider using the copy and swap idiom:

Student & assign(Student stu) // pass stu by value, constructing temp instance
{                             // this is the "copy" part of the idiom
    using namespace std;
    swap(*this, stu); // pass current values to temp instance to be destroyed
                      // and temp values to *this
    return *this;
} // temp goes out of scope thereby destroying previous value of *this

This approach is exception-safe if swap doesn't throw and it has two potential drawbacks:

  • if Student is part of a class hierarchy (has virtual members) creating a temporary instance will be more costly.

  • in the case of self assignment the implementation should be a no-op (or a self equality test), but in that case it will create an extra object instance. The self-assignment case is very rare.

utnapistim
  • 26,809
  • 3
  • 46
  • 82
  • If it is not a multithread, self assignment check is better than create a temporary instance everytime. Isn't? – Donglei Jul 05 '13 at 14:14
  • It is not better with self-assignment check. The code with copy&swap is minimal (there is no repetition at all), has no branches (you may get better performance with creating a copy but no code branches than passing by reference, depending on compiler optimizations), supports assignment from rvalue reference (if your `Student` can be constructed from it) and is exception safe. All this has nothing to do with multithreaded environments, though the implementation would have advantages there as well. – utnapistim Jul 05 '13 at 14:39