Code review comment for lp:~rryan/mixxx/atomic-co

Revision history for this message
Daniel Schürmann (daschuer) wrote :

Hi RJ,

Here only some edges in advanced, not a complete review.

* Since ControlObjectBase is not a base class any more, we should rename it to ControlObjectAtomic
* the name ControlNumericPrivate is fine for now, but if we want to finaly allow other numeric types, this should be more specific. What about ControlDoublePrivate?
http://mixxx.org/wiki/doku.php/revamped_control_system#control_object_types
* I am not sure if I like your m_pBehavior logic. It looks like you intended to make it also non blocking thread save
But I think it is not required, because only the CO owner should be allowed to set the behaviour of the CO.
It is required to change from elsewhere?
I think your solution might be not entirely save, because this check may fail:
return m_pBehavior ? m_pBehavior->defaultValue(default_value) : default_value;
m_pBehavior may change to NULL or from NULL between the check and the use.
ControlObjectBase<ControlNumericBehaviour> would be a solution but again I don't think it is required.
* We schould check the same for

« Back to merge proposal