I wrote up this code after reading item 11 of Effective C++ ( Third Edition ).
我在阅读了Effective C ++(第三版)的第11项后写了这段代码。
#include <iostream>
using namespace std;
#define MAX_COLORS 20
class Widget
{
public:
Widget ( int seed );
~Widget ( );
Widget& operator=( const Widget& rhs );
void ToString ( );
private:
Widget& SelfAssignmentUnsafe ( const Widget& rhs );
Widget& SelfAssignmentSafe ( const Widget& rhs );
Widget& SelfAssignmentAndExceptionSafe ( const Widget& rhs );
void MakeDeepCopy ( const Widget& rhs );
int *colorPallete;
};
void Widget::ToString()
{
int i = 0;
for ( i = 0; i < MAX_COLORS; i++ )
{
cout << "colorPallete[" << i << "]: " << colorPallete[i] << endl;
}
}
Widget::Widget ( int seed ):
colorPallete ( new int[MAX_COLORS])
{
int i = 0;
for ( i = 0; i < MAX_COLORS; i++ )
{
colorPallete[i] = seed + i;
}
}
Widget& Widget::operator=( const Widget& rhs )
{
// return SelfAssignmentUnsafe ( rhs );
// return SelfAssignmentSafe( rhs );
return SelfAssignmentAndExceptionSafe ( rhs );
}
Widget& Widget::SelfAssignmentUnsafe ( const Widget& rhs )
{
delete[] colorPallete;
colorPallete = 0;
MakeDeepCopy( rhs );
return *this;
}
Widget& Widget::SelfAssignmentSafe ( const Widget& rhs )
{
if ( this == &rhs ) return *this;
delete[] colorPallete;
colorPallete = 0;
MakeDeepCopy ( rhs );
return *this;
}
void Widget::MakeDeepCopy ( const Widget& rhs )
{
int i = 0;
colorPallete = new int [MAX_COLORS];
for ( i = 0;i < MAX_COLORS; i++ )
{
colorPallete[i] = rhs.colorPallete[i];
}
}
Widget& Widget::SelfAssignmentAndExceptionSafe ( const Widget& rhs )
{
int *origColorPallete = colorPallete;
MakeDeepCopy ( rhs );
delete[] origColorPallete;
origColorPallete = 0;
return *this;
}
Widget::~Widget()
{
delete[] colorPallete;
}
int main()
{
Widget b(10);
Widget a(20);
b.ToString();
b = b;
cout << endl << "After: " << endl;
b.ToString();
}
The author talks about handling assignment to self in the assignment operator:
作者讨论了在赋值运算符中处理对self的赋值:
Widget a(10);
a = a;
From the assignment operator for Widget I call Widget::SelfAssignmentAndExceptionSafe.
从Widget的赋值运算符我调用Widget :: SelfAssignmentAndExceptionSafe。
In Widget::SelfAssignmentAndExceptionSafe the idea is to save the colorPallete pointer in origColorPallete. Then make a deep copy of rhs.colorPallete. When the copy succeeds I delete the original pointer and return reference to self.
在Widget :: SelfAssignmentAndExceptionSafe中,我们的想法是将colorPallete指针保存在origColorPallete中。然后制作rhs.colorPallete的深层副本。当复制成功时,我删除原始指针并返回对self的引用。
The above mechanism is supposed to be self assignment and exception safe.
上述机制应该是自我分配和异常安全。
However, Widget::SelfAssignmentAndExceptionSafe is not able to handle assignment to self properly. The colorPallete array contains junk after self-assignment. Its handling the other cases very well.
但是,Widget :: SelfAssignmentAndExceptionSafe无法正确处理自我赋值。 colorPallete数组在自我赋值后包含垃圾。它非常好地处理其他情况。
Why could this be?
为什么会这样?
Please help.
[EDIT: After examining all the answers]
[编辑:审查了所有的答案后]
Thanks for your answers. I have updated the MakeDeepCopy method and the example's working fine now. Below, I have pasted the updated code:
谢谢你的回答。我已经更新了MakeDeepCopy方法,并且示例现在正常工作。下面,我已粘贴更新的代码:
#include <iostream>
using namespace std;
#define MAX_COLORS 20
class Widget
{
public:
Widget ( int seed );
~Widget ( );
Widget& operator=( const Widget& rhs );
void ToString ( );
private:
Widget( Widget& rhs );
Widget& SelfAssignmentUnsafe ( const Widget& rhs );
Widget& SelfAssignmentSafe ( const Widget& rhs );
Widget& SelfAssignmentAndExceptionSafe ( const Widget& rhs );
void MakeDeepCopy ( const int* rhs );
int *colorPallete;
};
void Widget::ToString()
{
int i = 0;
for ( i = 0; i < MAX_COLORS; i++ )
{
cout << "colorPallete[" << i << "]: " << colorPallete[i] << endl;
}
}
Widget::Widget ( int seed ):
colorPallete ( new int[MAX_COLORS])
{
int i = 0;
for ( i = 0; i < MAX_COLORS; i++ )
{
colorPallete[i] = seed + i;
}
}
Widget& Widget::operator=( const Widget& rhs )
{
// return SelfAssignmentUnsafe ( rhs );
// return SelfAssignmentSafe( rhs );
return SelfAssignmentAndExceptionSafe ( rhs );
}
Widget& Widget::SelfAssignmentUnsafe ( const Widget& rhs )
{
delete[] colorPallete;
colorPallete = 0;
MakeDeepCopy( rhs.colorPallete );
return *this;
}
Widget& Widget::SelfAssignmentSafe ( const Widget& rhs )
{
if ( this == &rhs ) return *this;
delete[] colorPallete;
colorPallete = 0;
MakeDeepCopy ( rhs.colorPallete );
return *this;
}
void Widget::MakeDeepCopy ( const int* rhs )
{
int i = 0;
colorPallete = new int [MAX_COLORS];
for ( i = 0;i < MAX_COLORS; i++ )
{
colorPallete[i] = rhs[i];
}
}
Widget& Widget::SelfAssignmentAndExceptionSafe ( const Widget& rhs )
{
int *origColorPallete = colorPallete;
MakeDeepCopy ( rhs.colorPallete );
delete[] origColorPallete;
origColorPallete = 0;
return *this;
}
Widget::~Widget()
{
delete[] colorPallete;
}
int main()
{
Widget b(10);
Widget a(20);
b.ToString();
b = b;
cout << endl << "After: " << endl;
b.ToString();
}
[EDIT: Modified code based on Charles's response ]
[编辑:根据Charles的回复修改代码]
The idea is to implement "copy-and-swap" idiom to make code both self assignment and exception safe. Note that copy is implemented only in the copy constructor. If the copy succeeds we swap in the assignment operator.
我们的想法是实现“复制和交换”习惯,使代码既可以自我赋值,也可以保证异常安全。请注意,复制仅在复制构造函数中实现。如果复制成功,我们交换赋值运算符。
Another improvement over the previous update is that MakeDeepCopy's interface depended on correct usage. We had to store/delete the colorPallete pointer before calling MakeDeepCopy. No such dependencies exist now.
与之前的更新相比,另一项改进是MakeDeepCopy的界面取决于正确的用法。我们必须在调用MakeDeepCopy之前存储/删除colorPallete指针。现在不存在这样的依赖。
#include <iostream>
using namespace std;
#define MAX_COLORS 20
class Widget
{
public:
Widget ( int seed );
~Widget ( );
Widget& operator=( const Widget& rhs );
void ToString ( );
Widget( const Widget& rhs );
private:
int *colorPallete;
};
void Widget::ToString()
{
int i = 0;
for ( i = 0; i < MAX_COLORS; i++ )
{
cout << "colorPallete[" << i << "]: " << colorPallete[i] << endl;
}
}
Widget::Widget ( int seed ):
colorPallete ( new int[MAX_COLORS])
{
int i = 0;
for ( i = 0; i < MAX_COLORS; i++ )
{
colorPallete[i] = seed + i;
}
}
Widget::Widget( const Widget& rhs ):
colorPallete( new int[MAX_COLORS] )
{
std::copy ( rhs.colorPallete, rhs.colorPallete + MAX_COLORS, colorPallete );
}
Widget& Widget::operator=( const Widget& rhs )
{
Widget tmp(rhs);
std::swap ( colorPallete, tmp.colorPallete );
return *this;
}
Widget::~Widget()
{
delete[] colorPallete;
}
int main()
{
Widget b(10);
Widget a(20);
b.ToString();
b = b;
cout << endl << "After: " << endl;
b.ToString();
}
5 个解决方案
#1
The junk you're seeing is because the MakeDeepCopy function always copies from the colorPallete member of rhs, instead of the copy you made in origColorPallete.
您看到的垃圾是因为MakeDeepCopy函数始终从rhs的colorPallete成员复制,而不是您在origColorPallete中创建的副本。
The following modification would fix it:
以下修改将解决它:
int *Widget::MakeDeepCopy ( const int *rhs )
{
int i = 0;
int *colorPallete = new int [MAX_COLORS];
for ( i = 0;i < MAX_COLORS; i++ )
{
colorPallete[i] = rhs[i];
}
return colorPallete;
}
Widget& Widget::SelfAssignmentAndExceptionSafe ( const Widget& rhs )
{
int *origColorPallete = colorPallete;
colorPallete = MakeDeepCopy ( origColorPallete );
delete[] origColorPallete;
origColorPallete = 0;
return *this;
}
Actually, with the above modification you may want to rename MakeDeepCopy to CopyColorPalette or something (particularly if you want to keep the original MakeDeepCopy for other purposes).
实际上,通过上述修改,您可能希望将MakeDeepCopy重命名为CopyColorPalette或其他东西(特别是如果您想将原始MakeDeepCopy用于其他目的)。
#2
You could avoid a lot of this hassle by simply using a std::vector instead of a dynamically allocated array. Vectors support assignment (including self assignment) so there would be nothing really to do.
通过简单地使用std :: vector而不是动态分配的数组,可以避免很多麻烦。向量支持赋值(包括自我赋值),因此没有什么可以做的。
#3
the problem is that you are not dealing with the copying on itself. Therefore, when you are performing a copy on itself, the statement
问题是你没有处理复制本身。因此,当您在自身上执行复制时,语句
colorPallete = new int [MAX_COLORS];
is actually also overwriting the colorPallete of rhs
实际上也覆盖了rhs的colorPallete
#4
When you call MakeDeepCopy you always pass in the reference to the object. Hence it again runs as a self-assignment.
调用MakeDeepCopy时,始终传入对象的引用。因此它再次作为自我指派运行。
You'll be much better off if you check against self-assignment in each public method and only run copying if assignment is called with another object passed.
如果在每个公共方法中检查自我赋值,并且只有在传递了另一个对象的情况下调用赋值时才会运行复制,那么你会好得多。
#5
Sticking out like a sore thumb in your example is the lack of a user-defined copy constructor. As you are providing a user-defined destructor and assignment operator, it's reasonable to reason that you might need a user-defined copy constructor and that is, indeed, the case here. Any explicit or implicit call to the compiler generated copy constructor will lead to undefined behaviour when the last of the original and the copy is destroyed.
在你的例子中像一个痛苦的拇指伸出来是缺乏用户定义的复制构造函数。当您提供用户定义的析构函数和赋值运算符时,可以合理地推断出您可能需要一个用户定义的复制构造函数,这确实就是这里的情况。对原始数据和副本的最后一个进行销毁时,对编译器生成的复制构造函数的任何显式或隐式调用都将导致未定义的行为。
You can write a trivial no-throw swap function for your class and it's fairly easy to write an exception neutral copy constructor. (Actually, I believe that it's trivial to write and reasonably easy to reason that it's exception neutral.) If you implement your assignment operator in terms of these two functions (the copy-and-swap idiom), you should find it a lot easier. In particular, you should find that the need for any checks for self-assignment should go away.
您可以为您的类编写一个简单的无抛出交换函数,并且编写异常中性复制构造函数相当容易。 (实际上,我认为编写并且相当容易推理它是异常中立的是微不足道的。)如果你根据这两个函数(复制和交换习语)实现你的赋值运算符,你会发现它更容易。特别是,您应该发现任何自我指派检查的需要都应该消失。
Edit:
Since your update you have made the Widget assignment operator exception safe. However, your design depends on the fact that you only have a single operation in the assignment operation that could possibly throw (the allocation of new memory) as the assigment of ints cannot throw. In general, if you held an array of objects this would not hold.
由于您的更新,您已使Widget赋值运算符异常安全。但是,您的设计取决于您在赋值操作中只有一个操作可能会抛出(新内存的分配)这一事实,因为int的赋值不能抛出。通常,如果你持有一个对象数组,这将无法容纳。
I understand that MakeDeepCopy is a private function, but even so it has an interface the depends heavily on correct usage. Either the member variable colorPallete must be delete[]ed and set to 0, or it must be saved to a temporary in the event that the call succeeds so that it can then be delete[]ed.
我知道MakeDeepCopy是一个私有函数,但即便如此,它有一个接口在很大程度上取决于正确的用法。成员变量colorPallete必须是delete [] ed并设置为0,或者在调用成功的情况下必须保存为临时变量,以便可以删除[] ed。
Even if you did not want to make a copy constructor public, I would still use it to implement the assignment operator as it makes the whole code simpler.
即使您不想将复制构造函数设置为public,我仍然会使用它来实现赋值运算符,因为它使整个代码更简单。
E.g.
Widget::Widget( const Widget& rhs )
: colorPallete( new int[MAX_COLORS] )
{
// OK because assigning ints won't through
std::copy( rhs.colorPallete, rhs.colorPallete + MAX_COLORS. colorPallete );
}
Widget& Widget::operator=( const Widget& rhs )
{
// Try allocating a copy, Widget's copy constructor must
// leak anything if it throws
Widget tmp( rhs );
// If that worked, swap with the copy - this can't throw
std::swap( colorPallete, tmp.colorPallete );
// Our old internals are now part of tmp so will be
// deallocated by tmp's destructor
}
I've got what's effectively your MakeDeepCopy in the copy constructor but without any of the necessary conditions on the calling code because it is a copy constructor and a simple two line assignment operator that is (IMHO) more obviously exception safe.
我在复制构造函数中有效地使用了MakeDeepCopy,但是在调用代码上没有任何必要条件,因为它是一个复制构造函数和一个简单的两行赋值运算符(IMHO)更明显是异常安全的。
Note that if you held an array of objects that might throw during assignment, you'd have to do something a bit cleverer to maintain exception safety and transparency. e.g. (and this probably illustrates why using a std::vector is a good idea):
请注意,如果您持有可能在分配期间抛出的对象数组,则必须做一些更聪明的事情来保持异常安全性和透明性。例如(这可能说明了为什么使用std :: vector是一个好主意):
template< class T >
class PartialArrayDeleter
{
public:
PartialArrayDeleter( T* p )
: p_( p ) {}
~PartialArrayDeleter()
{
delete[] p_;
}
void reset()
{
p_ = 0;
}
private:
T* p_;
};
Widget::Widget( const Widget& rhs )
: colorPallete( new Obj[MAX_COLORS] )
{
PartialArrayDeleter<Obj> del( colorPallete );
std::copy( rhs.colorPallete, rhs.colorPallete + MAX_COLORS. colorPallete );
del.reset();
}
Edit 2:
In case you think it's not relevant to consider objects other than int being assigned, then note that if you only consider the class that you have, reallocating is not strictly necessary during assignment. All widgets have the same amount of memory allocated in their constructor. A simple, efficient and exception safe assignment operator would be:
如果您认为考虑除了被分配的int以外的对象是不相关的,那么请注意,如果您只考虑您拥有的类,则在分配期间不必严格重新分配。所有小部件在其构造函数中分配的内存量相同。一个简单,高效且异常安全的赋值运算符将是:
Widget& Widget::operator=( const Widget& rhs )
{
for( size_t i = 0; i != MAX_COLORS; ++i )
{
colorPallete[i] = rhs.colorPallete[i];
}
return *this;
}
Self-assignment of ints is safe and as previous noted assignment of ints is also exception safe. (I'm not 100% sure but I don't think that std::copy is technically guaranteed safe for a self-assignment copy.)
int的自我分配是安全的,并且如前所述的int分配也是例外安全的。 (我不是100%肯定,但我不认为std :: copy在技术上保证对于自我分配副本是安全的。)
#1
The junk you're seeing is because the MakeDeepCopy function always copies from the colorPallete member of rhs, instead of the copy you made in origColorPallete.
您看到的垃圾是因为MakeDeepCopy函数始终从rhs的colorPallete成员复制,而不是您在origColorPallete中创建的副本。
The following modification would fix it:
以下修改将解决它:
int *Widget::MakeDeepCopy ( const int *rhs )
{
int i = 0;
int *colorPallete = new int [MAX_COLORS];
for ( i = 0;i < MAX_COLORS; i++ )
{
colorPallete[i] = rhs[i];
}
return colorPallete;
}
Widget& Widget::SelfAssignmentAndExceptionSafe ( const Widget& rhs )
{
int *origColorPallete = colorPallete;
colorPallete = MakeDeepCopy ( origColorPallete );
delete[] origColorPallete;
origColorPallete = 0;
return *this;
}
Actually, with the above modification you may want to rename MakeDeepCopy to CopyColorPalette or something (particularly if you want to keep the original MakeDeepCopy for other purposes).
实际上,通过上述修改,您可能希望将MakeDeepCopy重命名为CopyColorPalette或其他东西(特别是如果您想将原始MakeDeepCopy用于其他目的)。
#2
You could avoid a lot of this hassle by simply using a std::vector instead of a dynamically allocated array. Vectors support assignment (including self assignment) so there would be nothing really to do.
通过简单地使用std :: vector而不是动态分配的数组,可以避免很多麻烦。向量支持赋值(包括自我赋值),因此没有什么可以做的。
#3
the problem is that you are not dealing with the copying on itself. Therefore, when you are performing a copy on itself, the statement
问题是你没有处理复制本身。因此,当您在自身上执行复制时,语句
colorPallete = new int [MAX_COLORS];
is actually also overwriting the colorPallete of rhs
实际上也覆盖了rhs的colorPallete
#4
When you call MakeDeepCopy you always pass in the reference to the object. Hence it again runs as a self-assignment.
调用MakeDeepCopy时,始终传入对象的引用。因此它再次作为自我指派运行。
You'll be much better off if you check against self-assignment in each public method and only run copying if assignment is called with another object passed.
如果在每个公共方法中检查自我赋值,并且只有在传递了另一个对象的情况下调用赋值时才会运行复制,那么你会好得多。
#5
Sticking out like a sore thumb in your example is the lack of a user-defined copy constructor. As you are providing a user-defined destructor and assignment operator, it's reasonable to reason that you might need a user-defined copy constructor and that is, indeed, the case here. Any explicit or implicit call to the compiler generated copy constructor will lead to undefined behaviour when the last of the original and the copy is destroyed.
在你的例子中像一个痛苦的拇指伸出来是缺乏用户定义的复制构造函数。当您提供用户定义的析构函数和赋值运算符时,可以合理地推断出您可能需要一个用户定义的复制构造函数,这确实就是这里的情况。对原始数据和副本的最后一个进行销毁时,对编译器生成的复制构造函数的任何显式或隐式调用都将导致未定义的行为。
You can write a trivial no-throw swap function for your class and it's fairly easy to write an exception neutral copy constructor. (Actually, I believe that it's trivial to write and reasonably easy to reason that it's exception neutral.) If you implement your assignment operator in terms of these two functions (the copy-and-swap idiom), you should find it a lot easier. In particular, you should find that the need for any checks for self-assignment should go away.
您可以为您的类编写一个简单的无抛出交换函数,并且编写异常中性复制构造函数相当容易。 (实际上,我认为编写并且相当容易推理它是异常中立的是微不足道的。)如果你根据这两个函数(复制和交换习语)实现你的赋值运算符,你会发现它更容易。特别是,您应该发现任何自我指派检查的需要都应该消失。
Edit:
Since your update you have made the Widget assignment operator exception safe. However, your design depends on the fact that you only have a single operation in the assignment operation that could possibly throw (the allocation of new memory) as the assigment of ints cannot throw. In general, if you held an array of objects this would not hold.
由于您的更新,您已使Widget赋值运算符异常安全。但是,您的设计取决于您在赋值操作中只有一个操作可能会抛出(新内存的分配)这一事实,因为int的赋值不能抛出。通常,如果你持有一个对象数组,这将无法容纳。
I understand that MakeDeepCopy is a private function, but even so it has an interface the depends heavily on correct usage. Either the member variable colorPallete must be delete[]ed and set to 0, or it must be saved to a temporary in the event that the call succeeds so that it can then be delete[]ed.
我知道MakeDeepCopy是一个私有函数,但即便如此,它有一个接口在很大程度上取决于正确的用法。成员变量colorPallete必须是delete [] ed并设置为0,或者在调用成功的情况下必须保存为临时变量,以便可以删除[] ed。
Even if you did not want to make a copy constructor public, I would still use it to implement the assignment operator as it makes the whole code simpler.
即使您不想将复制构造函数设置为public,我仍然会使用它来实现赋值运算符,因为它使整个代码更简单。
E.g.
Widget::Widget( const Widget& rhs )
: colorPallete( new int[MAX_COLORS] )
{
// OK because assigning ints won't through
std::copy( rhs.colorPallete, rhs.colorPallete + MAX_COLORS. colorPallete );
}
Widget& Widget::operator=( const Widget& rhs )
{
// Try allocating a copy, Widget's copy constructor must
// leak anything if it throws
Widget tmp( rhs );
// If that worked, swap with the copy - this can't throw
std::swap( colorPallete, tmp.colorPallete );
// Our old internals are now part of tmp so will be
// deallocated by tmp's destructor
}
I've got what's effectively your MakeDeepCopy in the copy constructor but without any of the necessary conditions on the calling code because it is a copy constructor and a simple two line assignment operator that is (IMHO) more obviously exception safe.
我在复制构造函数中有效地使用了MakeDeepCopy,但是在调用代码上没有任何必要条件,因为它是一个复制构造函数和一个简单的两行赋值运算符(IMHO)更明显是异常安全的。
Note that if you held an array of objects that might throw during assignment, you'd have to do something a bit cleverer to maintain exception safety and transparency. e.g. (and this probably illustrates why using a std::vector is a good idea):
请注意,如果您持有可能在分配期间抛出的对象数组,则必须做一些更聪明的事情来保持异常安全性和透明性。例如(这可能说明了为什么使用std :: vector是一个好主意):
template< class T >
class PartialArrayDeleter
{
public:
PartialArrayDeleter( T* p )
: p_( p ) {}
~PartialArrayDeleter()
{
delete[] p_;
}
void reset()
{
p_ = 0;
}
private:
T* p_;
};
Widget::Widget( const Widget& rhs )
: colorPallete( new Obj[MAX_COLORS] )
{
PartialArrayDeleter<Obj> del( colorPallete );
std::copy( rhs.colorPallete, rhs.colorPallete + MAX_COLORS. colorPallete );
del.reset();
}
Edit 2:
In case you think it's not relevant to consider objects other than int being assigned, then note that if you only consider the class that you have, reallocating is not strictly necessary during assignment. All widgets have the same amount of memory allocated in their constructor. A simple, efficient and exception safe assignment operator would be:
如果您认为考虑除了被分配的int以外的对象是不相关的,那么请注意,如果您只考虑您拥有的类,则在分配期间不必严格重新分配。所有小部件在其构造函数中分配的内存量相同。一个简单,高效且异常安全的赋值运算符将是:
Widget& Widget::operator=( const Widget& rhs )
{
for( size_t i = 0; i != MAX_COLORS; ++i )
{
colorPallete[i] = rhs.colorPallete[i];
}
return *this;
}
Self-assignment of ints is safe and as previous noted assignment of ints is also exception safe. (I'm not 100% sure but I don't think that std::copy is technically guaranteed safe for a self-assignment copy.)
int的自我分配是安全的,并且如前所述的int分配也是例外安全的。 (我不是100%肯定,但我不认为std :: copy在技术上保证对于自我分配副本是安全的。)