[PATCH 10/25] mciseq: Limit concurrency when starting to play.

[Joerg-Cyril.Hoehle at t-systems.com]

Hi,

>+    switch (wmm->dwStatus) {
>+    case MCI_MODE_PLAY:
>+    case MCI_MODE_PAUSE:
>+	wmm->dwStatus = MCI_MODE_STOP;
>+    }

>This is weird. You're "missing" a break,
You're right, at least a /* fall through */ or break is custom.

>and it seems like an if statement would be more appropriate here, anyway.
I wouldn't like it.  dwStatus is to be considered like a volatile.
The Switch statements embeds the idea that the value is loaded once,
then dispatched upon.  The code does not depend on that (nor is the
C compiler prevented from reloading from memory without volatile declaration),
but that's my model.  A chain of If statements would not carry that idea.


That's why, in patch 20/25, I wrote:
+	if (wmm->dwStatus == MCI_MODE_STOP || wmm->dwStatus == MCI_MODE_NOT_READY)
+	    break;
+	else if (wmm->dwStatus != MCI_MODE_PLAY)
+	    continue;

From a pure logic point of view, it could have been more directly expressed as:
  else if (wmm->dwStatus != MCI_MODE_PLAY) break;
or
  if (wmm->dwStatus == MCI_MODE_PAUSE) continue;
  if (wmm->dwStatus != MCI_MODE_PLAY)  break;
but that's not the same.  A concurrent thread might
change dwStatus to PAUSE right between the 2 lines.

Unfortunately, within a Switch statement, Break cannot be used anymore
to get out of the surrounding While loop. So I either had to use Goto
or write the convoluted If statement.

The Ada language allows its Break statement to exit nested scopes.  These
are reliably referenced by name instead of by numeric nesting level like in Tcl.

Rethinking the issue, it could have been written as:
  else if (wmm->dwStatus != MCI_MODE_PLAY) continue;
when reintroducing the original loop condition:
 while (wmm->dwStatus != MCI_MODE_STOP && wmm->dwStatus != MCI_MODE_NOT_READY)
that still must *not* be expressed more readably as:
 while (wmm->dwStatus == MCI_MODE_PLAY || wmm->dwStatus == MCI_MODE_PAUSE)


>you introduce a scary magic number, 0x78B0
It's always been in the >10 year old code, just moved around.
Some Linux/ALSA header has symbolic names for the MIDI commands,
but only winealsa.drv/midi.c can use these.

Thank you,
	Jörg Höhle