Re: Must disable operator= for move to work right.
On 21/10/2010 00:20, Jim Langston wrote:
"Luc Danton" <lucdanton@free.fr> wrote in message
news:4cbf65e0$0$7761$426a74cc@news.free.fr...
On 20/10/2010 23:52, Jim Langston wrote:
"Luc Danton" <lucdanton@free.fr> wrote in message
news:4cbf6212$0$29778$426a74cc@news.free.fr...
On 20/10/2010 22:57, Jim Langston wrote:
The issue: use C++0x std::move for containers to allow me to add
non-copyable objects to maps.
The problem: my attempt to use the insert syntax of
myMap["two"] = foo(2);
didn't work and I had to use the syntax
myMap.insert( std::pair<std::string, foo>( "two", foo(2) ));
to get the expected result.
The following compilable in Microsoft Express 2010 demonstrates the
issue. Compiled as is we get the output line
2 foo Destructor
twice instead of only once as desired.
Has it always been a rule that a non-copyable class is non
assignable or
is this something that has just become a problem? Looking at the
code,
however, I really don't know why in the heck the program is moving 0's
around insted of 2's.
It's the Rule of Three, isn't it? If you somehow disabled the copy
constructor, no copy assignment operator was generated. More
generally, CopyConstructible is a distinct concept from
CopyAssignable. In C++0x there are MoveConstructible and
MoveAssignable also.
Supposedly if your value type is DefaultConstructible and
MoveAssignable, the line
myMap["two"] = foo(2);
should work (but I didn't check the requirements for the types in an
std::map).
The problem seems to be in the definition of "should work". I had
thought that I wouldn't have to disable the assignment operator. In the
worst case I thought I would get a bitwise copy which is fine, but for
some reason using that line of code causes the destructor to be called
twice on the "real" object when it should only be called once.
I guess my confusion is why the assignment operator breaks my code. I
can live with no assignment as I have the workaround.
If you disable the assignment operator, how can
myMap["two"] = foo(2);
work? If instead you use the insert syntax, and it only works with the
assignment operator disabled, I think you should check what concepts
your class satisfy. Types that are CopyAssignable but not
CopyConstructible strike me as a little odd. Are you sure value
semantics are appropriate?
When I construct the object I have opengl load the font, and I save the
id that opengl gives me for it. I only want to have the font destroyed
in opengl when I am done with it. A smart pointer would probably work
but I prefer to work without pointers if possible just because it
simplifies everything in the end and I don't have to worry about
ownership issues.
In this case I want the map to be the owner of the instances of foo and
only when I remove it from the map should it be removed from opengl.
With the move and insert this works. With move and assignment, it
doesn't. I do not know if this is a defect in my code (which is shown
here for all to see with the exact issue demonstrated) or a defect in
Express 2010 or a defect in C++0x or if this is the way it is, that
assignements must be disabled for this to work as advertised.
If I understand correctly, the raw type is a handle which has to be
disposed of with an appropriate function. This is reference semantics
where the handle acts as the reference. I'd go without deep copying
(since it is expensive) and without shallow copying (ref. counting
doesn't seem to be needed).
I'd personally use an std::unique_ptr because I hold the opposite
opinion as yours: it does simplify everything in the end. I find that
std::unique_ptr<handle, void(*)(handle*)> to give me the exact amount of
information I need, and no more. An appropriately named typedef takes
care of the verbosity.
Alternatively and equivalently since you're writing your own wrapper
class I recommend disabling copy construction and copy assignment
coupled with move construction/assignment that Does The Right Thing:
leave the moved from object in a consistent but destructible state.
Assuming the handles are ints and 0 is a special value for null objects,
the class would look like:
typedef int handle;
void dispose_font(handle);
class wrapper {
public:
// if you need default construction
explicit
wrapper(handle h_): h(h_) {}
wrapper(wrapper const&) = delete;
wrapper& operator=(wrapper const&) = delete;
wrapper(wrapper&& other)
: h(other.h)
{
other.h = 0;
}
// operator=(&&) works on the same principle
~wrapper()
{
// this assumes dispose_font(0) is safe/a no-op
dispose_font(h)
}
private:
handle h;
};
which is clearly simpler ;). If this is what your class already does,
then I'd double check the constructors/operator/destructor because you
shouldn't worry about how many times a destructor is called: every
object should always be in a consistent state allowing for safe
destruction! You have no guarantees (I think) on how many
objects/temporaries will be created and destroyed during calls to
map::operator[] or map::insert.
The simplest thing to (unit-)test would be creating a handle, move
constructing another handle from that, and then move assigning back to
the original handle. Only one font should be loaded (once) and then
disposed of (once).
Also have you tried stepping in a debugger to check that the objects are
in consistent states at destruction time? You could put a breakpoint in
the destructor to do this.