Re: Default move constructor/assignment
On Apr 7, 2:26 am, woodbria...@gmail.com wrote:
On Friday, April 6, 2012 12:29:05 AM UTC-5, Juha Nieminen wrote:
I'm glad that move constructors are implicitly generated by the
compiler at least on simple cases because if they weren't, that would
*severely* limit the usefulness of having them in the first place.
For example, you would benefit from move constructors when using eg=
..
a std::vector directly, but immediately when you put that std::vector
inside a simple struct, you would lose those benefits (unless you went
into the trouble of explicitly declaring the move constructor).
I didn't really get what kind of code do these implicit move constr=
uctors
break, but I'm pretty certain that the benefits greatly outweight the
disadvantages.
Yeah, I pretty much agree.
http://www.velocityreviews.com/forums/t730954-class-invariants-and-im...
I agree too. However it is good to be aware of the cases that do
break, so you can learn to recognize them, and learn how to fix them.
The most convincing case I've seen to date is this one:
http://groups.google.com/group/comp.lang.c++.moderated/browse_thread/thread=
/6b158b62e7debf05#
The code shown doesn't break. And the original code shown has
incorrect move members. But nevertheless it is a reasonable example
of code that might break when moving from C++03 to C++11. To break it
you have to dress it up a little more:
#include <cassert>
#include <vector>
#include <iostream>
typedef unsigned char Byte;
typedef std::vector<Byte> ImageVec;
class Image
{
ImageVec m_data;
size_t m_width;
size_t m_height;
public:
Image(): m_width(0), m_height(0) {}
Image(Byte* img_data, size_t width, size_t height):
m_data(img_data, img_data + width * height * 3),
m_width(width),
m_height(height) {
}
Byte& operator()(size_t i, size_t j, size_t k)
{
return m_data[3*m_width*i + 3*j + k];
}
friend
std::ostream& operator<<(std::ostream& os, const Image& img)
{
os << "{\n";
if (img.m_height > 0)
{
os << " {";
if (img.m_width > 0)
{
os << '{';
os << static_cast<unsigned>(img.m_data[0]) << ','
<< static_cast<unsigned>(img.m_data[1]) << ','
<< static_cast<unsigned>(img.m_data[2]);
os << '}';
for (size_t j = 1; j < img.m_width; ++j)
{
os << ",{";
os << static_cast<unsigned>(img.m_data[3*j]) <<
','
<< static_cast<unsigned>(img.m_data[3*j+1]) <<
','
<< static_cast<unsigned>(img.m_data[3*j+2]);
os << '}';
}
}
os << "}";
for (size_t i = 1; i < img.m_height; ++i)
{
os << ",\n {";
if (img.m_width > 0)
{
os << '{';
os <<
static_cast<unsigned>(img.m_data[3*img.m_width*i]) << ','
<<
static_cast<unsigned>(img.m_data[3*img.m_width*i+1]) << ','
<<
static_cast<unsigned>(img.m_data[3*img.m_width*i+2]);
os << '}';
for (size_t j = 1; j < img.m_width; ++j)
{
os << ",{";
os <<
static_cast<unsigned>(img.m_data[3*img.m_width*i+3*j]) << ','
<<
static_cast<unsigned>(img.m_data[3*img.m_width*i+3*j+1]) << ','
<<
static_cast<unsigned>(img.m_data[3*img.m_width*i+3*j+2]);
os << '}';
}
}
os << "}";
}
}
return os << "\n}\n";
}
friend
bool
operator==(const Image& x, const Image& y)
{
return x.m_width == y.m_width && x.m_height == y.m_height &=
&
x.m_data == y.m_data;
}
friend
bool
operator!=(const Image& x, const Image& y)
{
return !(x == y);
}
};
Image
get_Image()
{
Byte img_data[][6][3] =
{
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}}
};
return Image(reinterpret_cast<Byte*>(img_data), 6, 4);
};
int main()
{
std::vector<Image> v;
v.push_back(get_Image());
v.push_back(get_Image());
v.push_back(get_Image());
std::cout << v.back() << '\n';
v.back()(0, 0, 0) = 0;
std::vector<Image>::iterator j = std::unique(v.begin(), v.end());
assert(j == v.begin() + 2);
std::cout << v.back() << '\n';
}
The key to why this code breaks is:
1. It's ostream<<() friend assumes that m_data.size() == 3 * m_width
* m_height, and indeed that is a good assumption in C++03.
2. Client code actually inspects (prints out) a value that will be
moved from in C++11. It does this by calling std::unique on a
sequence of Images, which ends up moving from Images at the end of the
sequence (in order to "remove" duplicates).
In C++03 this program prints out:
{
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}}
}
{
{{0,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}}
}
But when recompiled for C++11, it prints out (for me):
{
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}}
}
{
Segmentation fault: 11
Now if main() had used unique in the idiomatic way:
v.erase(std::unique(v.begin(), v.end()), v.end());
there would have been no crash. It is only because main used the
moved-from value at v.back() in a way that required Image's invariant
to hold that causes the crash.
The easiest way to fix this is to default the copy members:
Image(const Image&) = default;
Image& operator=(const Image&) = default;
Just doing this inhibits auto-generation of the move members, and then
the code again prints out:
{
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}}
}
{
{{0,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}}
}
If you want to properly add the move members, you can't default them,
as the defaults break the invariant as discussed above. The proper
move members must set the source to the default constructed
(resourceless) state:
Image(Image&& img)
: m_data(std::move(img.m_data)),
m_width(img.m_width),
m_height(img.m_height)
{
img.m_data.clear();
img.m_width = 0;
img.m_height = 0;
}
Image& operator=(Image&& img)
{
m_data = std::move(img.m_data);
m_width = img.m_width;
m_height = img.m_height;
img.m_data.clear();
img.m_width = 0;
img.m_height = 0;
return *this;
}
And now the code prints out:
{
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}},
{{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3},{1,2,3}}
}
{
}
Howard