[PATCH 01/13] dsound: New sample rate converter core functions. (resend)

[Krzysztof Nikiel]

2011/2/11 Vitaliy Margolen <wine-devel at kievinfo.com>:
> On 02/11/2011 04:34 AM, Krzysztof Nikiel wrote:
>>
>> +typedef double sample_t;
>>
>> +static inline sample_t getsample(LPBYTE buf, INT bps)
>> +{
>> +        tmp = ((sample_t) (*((BYTE *) buf)) - 128.0);
>
> No need to type cast. "buf" already a pointer to the BYTE. You can use it
> directly as *buf or buf[0], which is even more readable.

Indeed, but I think it would be even better to change 'buf' type to
void* as the actual size/type is defined by 'bps'.

>
>> +static inline void putsample(LPBYTE buf, INT bps, sample_t smp)
>> +        CLIPSAMPLE(smp, (double) 0x7fff);
>
> Typecasts are bad, especially that the type of "smp" is not "double". Please
> use "32767.0" instead.

So maybe something like (sample_t)0x7fff would be better or better yet
move the typecast to the macro definition.

>
>> +    DOUBLE amp[3];
>
> Mixing types again. If you defined your own type then please use it. Or make
> everything the same type whichever it is.

I'm not quite sure what type it should be, 'sample_t' doesn't look
quite good to me but maybe you're right.

>
>
>> [PATCH 02/13] dsound: Resample filter table generator. (resend)
>
> Don't think compiler helpers should be going into dll directories. The more
> correct place is tools. But it's up to AJ.

Ah, I see.

>
>
> And as others noted, your patch series is not split correctly. Each patch
> will break Wine tree until the very last one. This is against the patch
> requirements that code tree should compile after every commit.
>
> You can put your compile helper with it's make file first. Then add your
> resampling code with a make file changes to compile it. But not use it yet
> (it does break another rule for adding dead code, but that's the only way
> you can split the entire series). After that make use of your new resampler,
> removing all conflicting code, so Wine will work. Last commit can be a
> cleanup for all remaining pieces that are no longer used.

Sounds reasonable, but not quite easy to do. It looks like wine will
use old code for some time until I try to do the correct split.
Thanks.

>
> Vitaliy.
>