logo标签列表

这段C++代码包含哪些未定义行为？

c++pointers
4

我在阅读《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();
}

作者谈论了在赋值运算符中如何处理自我赋值的问题：
Widget a(10);
a = a;

我从Widget的赋值运算符中调用Widget::SelfAssignmentAndExceptionSafe。

在Widget::SelfAssignmentAndExceptionSafe中，想法是将colorPallete指针保存在origColorPallete中。然后对rhs.colorPallete进行深度复制。当复制成功时，删除原始指针并返回对自身的引用。

上述机制应该是自我赋值和异常安全的。

然而，Widget::SelfAssignmentAndExceptionSafe不能正确处理对自身的赋值。自我赋值后，colorPallete数组包含垃圾数据。它很好地处理了其他情况。

为什么会这样呢？

请帮忙解决。

[编辑：检查了所有答案后]

感谢您的回答。我已更新了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();
}

[编辑：根据Charles的回复修改了代码]

这个想法是实现“复制并交换”惯用语，使代码既可以进行自我分配，又可以进行异常安全。请注意，仅在复制构造函数中实现复制。如果复制成功，我们会在赋值运算符中进行交换。

与上次更新相比的另一个改进是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();
}
请提供能够编译并演示问题的示例代码。 - Greg Hewgill
拼写是“palette”：一个L，两个T。 - Rob Kennedy
糟糕！我将永远保持失读症。 - ardsrk
5个回答
1
你看到的垃圾是因为MakeDeepCopy函数总是从rhs的colorPallete成员复制，而不是你在origColorPallete中所做的复制。
以下修改将解决此问题：
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;        
}

实际上，通过以上修改，您可能希望将MakeDeepCopy重命名为CopyColorPalette或其他名称（特别是如果您想保留原始的MakeDeepCopy以供其他用途）。
但是在自我赋值的情况下，origColorPallete和rhs.colorPallete是相同的。正确吗？ - ardsrk
@ardsrk: 那又怎样？一个新的调色板被建立，旧的调色板正在被删除，但最终（除了指向旧调色板的指针被破坏的事实之外），对象仍然具有一个有效的调色板，并且上面的颜色是正确的。虽然效率低下，但是正确性比效率更重要。 - David Rodríguez - dribeas
Greg，你的版本根本没有使用rhs。 - Rob Kennedy
@Rob Kennedy：你说得对。既然OP已经找到了问题，我可以说我故意留下了那个错误来帮助理解问题。实际上，我只是犯了一个错误。 :) 我会保持原样。 - Greg Hewgill
1
问题在于你没有处理复制本身的情况。 因此，当你对自身执行复制操作时，会出现以下语句。
colorPallete = new int [MAX_COLORS];

实际上也覆盖了rhs的colorPallete

1
当你调用MakeDeepCopy时，你总是传递对象的引用。因此它再次作为自我赋值运行。
如果在每个公共方法中检查自我赋值，并且仅在调用分配时传递另一个对象时运行复制，那么情况会好得多。
1
如果你依赖于检查自我分配以外的任何东西，那么通常被认为是一种脆弱的设计，除了性能。 - David Rodríguez - dribeas
我真的不明白这怎么会导致脆弱的设计。我会将深拷贝方法设为私有并在所有相关的公共方法中检查自我赋值。简洁明了。 - sharptooth
@dribeas：我想知道为什么。用相同的内容覆盖一张纸是没有意义的，那么为什么你会对对象这样做呢？ - xtofl
1

您可以通过使用std::vector而不是动态分配数组来避免许多麻烦。向量支持赋值（包括自我赋值），因此实际上没有什么要做的。

尼尔，我可以做到。我只是想知道涉及的细微差别。 - ardsrk
0
在您的示例中，缺少用户定义的复制构造函数这一点异常突出。由于您提供了用户定义的析构函数和赋值运算符，因此合理推断您可能需要一个用户定义的复制构造函数，在这里确实需要这样做。任何对编译器生成的复制构造函数的显式或隐式调用都将导致在原始对象和其副本中最后一个销毁时未定义的行为。
您可以为类编写一个微不足道的 无抛出 交换函数，并且编写 异常中性 复制构造函数相当容易。（实际上，我认为编写它很简单，而且很容易推断它是异常中性的。）如果您在这两个函数（复制-并-交换惯用语）的基础上实现赋值运算符，您应该会发现它变得更加容易。特别是，您应该发现不需要检查自赋值的需求。 编辑: 自您更新以来，您已使 Widget 赋值运算符具有异常安全性。但是，您的设计取决于只有一个赋值操作可能引发异常（新内存分配），因为 int 的分配不会引发异常。一般而言，如果持有对象数组，则不会满足这一点。

我知道MakeDeepCopy是一个私有函数，但即使如此它仍然有一个界面，这个界面非常依赖于正确的使用。成员变量colorPallete必须被delete[]并设置为0，或者在调用成功后，必须将其保存到临时变量中，以便可以随后进行delete[]。

即使您不想公开复制构造函数，我仍然会使用它来实现赋值运算符，因为它可以让整个代码更简单。

例如：

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
}

我在复制构造函数中实现了类似于你的MakeDeepCopy的功能，但是没有必要在调用代码上添加任何必要的条件，因为它是一个复制构造函数和一个简单的两行赋值运算符，这样更容易理解并且更安全。

请注意，如果您持有可能在赋值过程中抛出异常的对象数组，则需要做一些更聪明的事情来保持异常安全性和透明度。例如（这也许说明了为什么使用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();
}

编辑2：

如果您认为除了分配给int之外的对象不相关，则请注意，如果您仅考虑您拥有的类，则在分配期间不严格需要重新分配。所有小部件在其构造函数中分配了相同数量的内存。一个简单，高效和异常安全的赋值运算符将是：

Widget& Widget::operator=( const Widget& rhs )
{
    for( size_t i = 0; i != MAX_COLORS; ++i )
    {
        colorPallete[i] = rhs.colorPallete[i];
    }
    return *this;
}

自赋值int是安全的，如前所述，将int赋值也是异常安全的。（我不100%确定，但我不认为std：：copy对于自赋值拷贝在技术上是保证安全的。）
Charles，我认为在这个例子中不需要复制构造函数。我将复制构造函数声明为私有，并没有提供定义。但是代码编译通过了。我的意图是在Widget::SelfAssignmentAndExceptionSafe中实现复制并交换惯用法。 - ardsrk
好的，但是你发布的代码没有包含一个私有声明的复制构造函数，因此 - 如所写 - 即使它不影响示例主要用途，你发布的Wibble也不够健壮。复制构造函数的优点是您保证使用全新的对象。这意味着您不必担心对象的“旧”状态。如果您尝试在复制构造函数之外的任何地方实现“复制”部分的“复制和交换”，则会遇到您所遇到的各种复杂情况。 - CB Bailey
网页内容由stack overflow 提供, 点击上面的
可以查看英文原文，
原文链接