On Wed, 2011-12-21 at 23:39 +0100, Maarten Bosmans wrote: > This commit: > http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=dc8edf4b43bf20e9edf6efd31836218924fad1f0 > > resampler: Move some peak resampler asserts around > This moves a couple of asserts from peak_resample() to peaks_init() > since they're resampler parameters that shouldn't change after > initialisation. > > is wrong IMHO. > > The whole point of asserts is to protect from programmer errors. This > can be null pointers (the other asserts in that function), but also > parameters that can have only certain values. The fact that the assert > can also be placed at init time is irrelevant. As the commit message > says, they _shouldn't_ change after init, the assert just makes sure > that they really haven't. That doesn't make sense to me. You add asserts to catch common errors (the wrong format was picked for the resampler) not basic semantics violations (format was changed after the resampler was instantiated). > Furthermore, the assert also functions as a code comment. As further > down in the function there is code like: > > if (r->work_format == PA_SAMPLE_S16NE) { > ... > } else { > ... > } > > With the assert > pa_assert(r->work_format == PA_SAMPLE_S16NE || r->work_format == > PA_SAMPLE_FLOAT32NE); > in the same function, it is clear that the else block handles the > float32 part, without the assert, that fact is hidden from the > programmer. > > I consider this second documentation aspect of the assert as least as > important as the correctness aspect of it. Again, I disagree. If code needs documentation, it should be documented. Relegating that to an assert is incorrect. > It would be nice if patches like this could be submitted to the > mailing list first, instead of committing directly. That way they can > gather feedback. If there's no reaction within a week or so, just > commit it. Well, for changes that I think would raise objections, I do post to the list. I'm really not for sending small patches like this one out to the list and wait a week for comments. Active participants such as yourself have the option to join the -commits list (perhaps you have already), and I think this gives us the ability to have a decent amount of review without adding an extra step. Cheers, Arun
