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

Alexandre Julliard julliard at winehq.org
Mon Oct 8 11:26:30 CDT 2012


<Joerg-Cyril.Hoehle at t-systems.com> writes:

> 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.

If the code depends on that sort of thing then it's broken. If another
thread can change dwStatus you need a critical section or an interlocked
function.

-- 
Alexandre Julliard
julliard at winehq.org



More information about the wine-devel mailing list