Re: [PATCH 01/13] dsound: New sample rate converter core functions.
2011/2/11 Dmitry Timoshkov <dmitry(a)codeweavers.com>:
Krzysztof Nikiel <knik00(a)gmail.com> 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.
2011/2/11 Krzysztof Nikiel <knik00(a)gmail.com>
2011/2/11 Dmitry Timoshkov <dmitry(a)codeweavers.com>:
Krzysztof Nikiel <knik00(a)gmail.com> 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.
still, what dmitry told you still holds. wine needs to be able to compile and pass the tests after each patch is applied. at least the makefile changes should be merged with the previous patches that needs them (1 and 2). the rest seems like fine splitting, although i don't know the code. what i'm not sure is, if resampler is still not called after the first patches, if it is fine to send those first patches... i think you also need to call resampler for the patch to be accepted, even if it is just as a stub, incomplete version.
2011/2/11 Ricardo Filipe <ricardojdfilipe(a)gmail.com>:
2011/2/11 Krzysztof Nikiel <knik00(a)gmail.com>
2011/2/11 Dmitry Timoshkov <dmitry(a)codeweavers.com>:
Krzysztof Nikiel <knik00(a)gmail.com> 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.
still, what dmitry told you still holds. wine needs to be able to compile and pass the tests after each patch is applied. at least the makefile changes should be merged with the previous patches that needs them (1 and 2). the rest seems like fine splitting, although i don't know the code. what i'm not sure is, if resampler is still not called after the first patches, if it is fine to send those first patches... i think you also need to call resampler for the patch to be accepted, even if it is just as a stub, incomplete version.
It's not that easy, I don't think it would be just incomplete, it would be broken. The original code is rather messy, especially mixer.c, no wonder no one wanted to touch it to fix the really poor sample rate conversion.
Hi Krzysztof, Op 11-02-11 12:16, Krzysztof Nikiel schreef: > 2011/2/11 Dmitry Timoshkov<dmitry(a)codeweavers.com>: >> Krzysztof Nikiel<knik00(a)gmail.com> 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
2011/2/11 Maarten Lankhorst <m.b.lankhorst(a)gmail.com>:
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..
Thanks, now I have at least two reasonable advices how to proceed with the split. It looks like the actual split may be more difficult to make than the patch itself :P
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.
Yes, 'tools' seems a good place to put it. I think if someone is really inclined to know where 'firtab.h' comes from it won't be a real problem to find it. I'm not sure what "out of tree" means but I think it does work out of tree. Anyway, if you like the unwrapped version, just visit the bugzilla and download the .zip pack. http://bugs.winehq.org/show_bug.cgi?id=14717
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.
I expected it will be a problematic patch. It's quite big and the original code is rather messy, especially mixer.c. I really doubt the patch will be easy to read even when split as you say.
Cheers, Maarten
participants (3)
-
Krzysztof Nikiel -
Maarten Lankhorst -
Ricardo Filipe