Re: [PATCH v9 0/10] MR4982: Load and play MIDI

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/dmime/midi.c:
> -
>    if (!(event = calloc(1, sizeof(*event)))) return E_OUTOFMEMORY;
> -
> -
>    hr = read_midi_event(stream, event, last_status, &bytes_read);
> -
>    if (hr != S_OK) break;
> -
>    list_add_tail(&This->events, &event->entry);
> -
>    This->event_count++;
> -
>    last_status = event->status;
> -
>    is_end_of_track = event->status == 0xff && event->type == MIDI_META_END_OF_TRACK;
> -
>    event = NULL;
> - }
> -
> - if (hr == S_OK && bytes_read != length)
> - {
> -
>    WARN("Read %lu bytes, expected %lu\n", bytes_read, length);
> -
>    hr = E_FAIL;
> - }

Do we really need this check (and the additional read_accumulate helper it requires)?

I understand how it might seem important for correctness, but checks need to be preferably either trivial (like parameter checks), or something that Windows does and that applications would need.

Ideally, any check should have a corresponding test to make sure it can be added without making the behavior different (let's imagine applications with bogus MIDI files and Windows maybe just parses whatever it can from them).