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

Sorry for taking so long to get you feedback :( better late than never?

I've skimmed the whole thing once but for now will just comment on controlobjectbase.h:

* static const int cReaderSlotCnt
  - Should not be static.. if multiple object files include controlobjectbase.h then we will get duplicate symbols during linking. This builds fine now because only controlobject.o includes it. You could just leave it as a const int.

* sizeof(T) <= sizeof(void*) is not enough to determine atomicity -- for example, if the pointer to a value is not memory-aligned then that breaks atomicity guarantees. I think to be sure we need to use a compiler directive in the ATOMIC=true template to request m_value is aligned.

* Need comments on all methods declarations in controlobjectbase.h. This is such a crucial file it needs plenty of documentation for future developers :).

* ControlObjectRingValue::tryGet()
  - why return an int when it's functionally a bool (i.e. was it successful)
  - bool originalSlots = m_readerSlots.fetchAndAddAcquire(-1);
  Should instead be:
    bool originalSlots = m_readerSlots.fetchAndAddAcquire(-1) <= 0;

The reason is this: m_readerSlots can go negative if too many concurrent reads occur simultaneously. It can even go negative if a write is happening and two reads happen right after the write has swapped m_readerSlots with the value 0 (the second read will see -1). So to detect whether a read should be successful, you should check if the value is <= 0.

  - Why is cReaderSlotCnt the number of concurrent readers allowed also the size of the ring buffer? I can't think of a reason why those 2 values need to be the same.
  - How did you choose 7?

* ControlObjectValue::getValue
  - Can you use ARRAYSIZE on m_ring instead of hard-coding cReaderSlotCnt+1 ? It should be compile-time.
  - When getValue() fails to read a slot, it goes backward one. This means we sacrifice consistency for speed. We need to make sure people understand this is a trade-off that is part of the control system. Under high load, the control system will possibly serve a stale result when you get().