No copy or move in NanCallback and NanAsyncWorker

[kkoopa]

�Fixes #330
Made the same changes to NanAsyncWorker, since it too contains a Persistent<T>.
PTAL

[bnoordhuis]

Instead of declaring the copy/move constructors and operators by hand, perhaps it's better to capture the logic in a NAN_DISALLOW_COPY_MOVE_ASSIGN macro, with one commit introducing the macro and updating existing declarations, and a second commit to update NanAsyncWorker.

[kkoopa]

Seems reasonable. This code exists in three places already, and I know there are some other classes (maybe NanAsciiString and friends) that prohibit some form of copying and assignment. I'll do that later today.

[agnat]

Not sure about this.

Maybe it has been discussed elsewhere but isn't this a perfect application for a persistent with copy semantics?

Non-copyable objects are so... awkward.

[bnoordhuis]

Maybe it has been discussed elsewhere but isn't this a perfect application for a persistent with copy semantics?

One problem with copyable persistent handles (at least, how they are currently implemented in V8), is that only the storage cell is cloned, not the auxiliary data.

You can, for example, copy a weak persistent handle but the resulting handle will be strongly persistent. That may be a non-issue for NanCallback but it's a big headache in general. Non-copyable is just easier to reason about.

[kkoopa]

_NanWeakCallbackInfo, NanCallback, NanAsyncWorker and NanAsyncProgressWorker all contain Persistent members. Out of these, only NanAsyncProgressWorker is non-copyable, non-assignable and non-movable at the moment. Additionally, NanAsciiString, NanUtf8String and NanUcs2String prohibit copying and assignment but not moving.

Should all of these be made to disallow copy, assign and move?

[bnoordhuis]

The default v8::Persistent is non-copyable in io.js and node.js v0.12 so their containers should be effectively non-copyable either.

Apropos move semantics, I think it should be alright to allow that but I haven't tested and I'm not 100% sure.

[kkoopa]

It was you who originally suggested disallowing move for NanAsyncProgressWorker https://github.com/iojs/nan/pull/197/files#r19300699 but maybe that was because of libuv.

[kkoopa]

There.

[bnoordhuis]

I think in that particular case it was to make sure that ExecutionState doesn't outlive the scope of the Execute callback. That's probably not a consideration for v8::Persistent. At any rate, I don't mind taking a reactive approach and keep everything non-moveable until someone complains.

[bnoordhuis]

LGTM with two suggestions.

[kkoopa]

Done. I'll leave this open for a while.

[bnoordhuis]

LGTM with one more suggestion.

[kkoopa]

Thanks. Merging,