Re: [PATCH 01/13] dsound: New sample rate converter core functions.

[Maarten Lankhorst]

Hi Krzysztof,

Op 11-02-11 12:16, Krzysztof Nikiel schreef:
> 2011/2/11 Dmitry Timoshkov:
> > Krzysztof Nikiel  wrote:
> > > > You can't send Makefile changes separately from added/removed
> > > > files, a patch should not add dead code.
> > >
> > > Could you explain "dead code", all 13 parts need to be applied,
> > > otherwise the code will be broken.
> >
> > Wine should compile and be able to pass 'make test' after each separate patch.
> > You can't add for instance dlls/dsound/resample.c in one patch, and add it to
> > Makefile or use interfaces provided by it in some later patches. Every patch
> > should be finished and self-containing.
>
> Well, previous version of this patch was rejected as "needs
> splitting", it's basically too big to be send as a single patch. It
> can be applied as several smaller chunks or rejected as a whole. I
> don't think there is any other option.

Thanks for looking at the format conversion functions. The splitting can 
probably be done in 4 parts:
1. If you have any generic bugfixes, put them in the first few patches. 
Once those are in you can look at the other parts. Each bugfix should 
probably be its own separate patch.
2. Single patch to remove the old behavior. At this point you probably 
only want the mixing to work without requiring reconversion, and have a 
FIXME or something to say rate conversion is unavailable.
3. A bunch of patches implementing new rate conversion, one logical step 
at a time. The only requirement is that you want to be able to build 
wine without it failing, and if possible even running directsound, even 
if the result is insane..

Having mkfir in the way you put it there will probably not work, since 
some people will build wine out of tree. My guess is that you should put 
that in tools. Since the contents of the file won't change often it may 
just be enough to have the .c file there and have a note in the mkfir.h 
header that it was generated by tools/mkfir.c, but I'm not AJ so he 
would have to decide.

The reason for this kind of splitting is that even if the code is broken 
between the removal of the old code and the last patch, it's a lot 
easier to see bugs. Usually in a patch like this you will spot 2 or 3 
bugs you wouldn't have otherwise spotted because you look at the code in 
logical parts. This also makes bisecting a lot easier. Otherwise you 
know what commit caused the regression, but you have no idea what part 
of the code is.

Cheers,
Maarten