Fix ExecutableNode::Task comparison functions

[andrewkaufman]

I'm migrating this from #109. See #838 as well for more discussion.

• Reconsider Task::hash() - why does it not use Executable::hash()? What is going on with the hashing of the node pointer?
• Fix Task::operator < . It has a return type of bool but the implementation code is returning -1, 1 or 0. Or rethink this entirely. Do we need this operator at all? Is hash() enough, because == and < are defined for hashes?
• Consider making immutable ExecutableNode::Task as suggested in the todo.

[andrewkaufman]

I tried the first suggestion and all my tests still passed, so that seems like a simple change.

It turns out the reason we need Task::operator < is so tasks can be inserted into the TaskDescription::requirements set by Dispatcher::uniqueTask, so rather than remove it completely I think I'll re-implement it in terms of Task::hash() as the todo in 5d50415 suggests.

[andrewkaufman]

About making it all const, I've ran into a few issues:

• Using const ContextPtr context or const ConstContextPtr context causes issues with Task() and Task( const Task & ) complaining that non-static const members can't use default assignment operator. Those constructors are required by Dispatcher::TaskDescription() and any place a task is added to a container, as in ExecutableNode::executionRequirements.
• Using ConstContextPtr context caused issues in the bindings, which made me notice that both node and context are currently bound as properties with are editable. setContext and setNode are defined, though not used in the tests. Are these supposed to be here? Is there a way to bind these properties as read-only instead?
• I have some code in Dispatcher which assumes it can edit the context of a task after creating it. This was just an optimization to avoid some extra ref counting, so I can work around it if necessary.

So my questions is, do you have a quick solution for me? If it'll take a bit of thought, I think I'd rather leave the immutable part as a todo, and consider the other comparison function changes enough to close this issue.

[johnhaddon]

First off, I realise I didn't specify this in the todo, but I would assume that as part of making the Task immutable, we'd also explicitly store the hash as a member variable, computing it only once at construction time. That would avoid potentially expensive recomputing every time == or < is called, which could be a lot given that they're used by the containers. It also ensures that the result of == or < won't change over time, which would break containers of Tasks anyway.

So, given that we want to compute the hash once on construction, we also want to ensure that the Context and node we store don't change afterwards, because that would invalidate the hash - that's where the necessity for constness comes in. Sorry if that's all obvious - I felt that I should clarify things given that I'd omitted to specify that I think the hash should be stored as a member as well.

If we make Task.context const ConstContextPtr, then logically assignment shouldn't be allowed - we've said that the member is const (it's the const and not the ConstPtr the compiler will be upset about) so the compiler is refusing to overwrite it behind our backs. If we wanted to keep it const and still have assignment, then we'd need to implement operator = ourselves, doing a cheeky cast internally. I don't think that would be entirely unreasonable, because our reason for constness is to keep hash, node and context in lockstep, and we'd still be doing that in operator =. The alternative is to make a private ConstContextPtr m_context and provide aConstContext *context()` accessor.

Boost::Python doesn't let you return ConstPtr (or const *) to Python, because Python has no concept of constness - so it just refuses, to warn us that we need to think about things. In several places in the Gaffer bindings (TypedObjectPlug::getValue() for one) I've returned a copy in such cases, on the grounds that pesky Python scripters (i.e. me) can't be trusted not to meddle with such things. I'd suggest we do the same thing here, but it would seem a bit odd for a direct attribute access like task.context to return a copy rather than a reference - would a task.context() call seem less off it returned a copy? If so, that seems to be two votes for an accessor rather than a publicly accessible field.

I think you can bind properties as read-only simply by omitting the set argument to the add_property call.

I don't mind deferring the immutability question for now, in the interests of getting dispatchers out there in the wild, but I'd like it to be addressed before we consider this milestone complete. Perhaps for now we should at least remove the setter from the Python property, since it's not in use - that would at least stop us introducing new code that might be affected by later changes.

[andrewkaufman]

I could probably bash out the private ConstPtr version with public accessors. That way python context() functions would match the c++, and not being settable would be obvious. I'll do this work on top of my (hopefully) soon to be merged frame range branch #884, since it will touch a bunch of similar code.

[andrewkaufman]

Can I do the same copy trick for the node() or should I just return the cast value there?