The patch http://git.xiph.org/?p=flac.git;a=commitdiff;h=0bea5fb96436588b4e78c5be79bf174b9be7a202 changes the type of t value from FLAC__int32 to uint32_t, but only for 24-bit signed input samples. 24-bit *un*signed input still uses FLAC__int32 t. So I wonder - does it makes sense to change this type too? The patch is attached. -------------- next part -------------- A non-text attachment was scrubbed... Name: uint_24.patch Type: application/octet-stream Size: 678 bytes Desc: not available Url : http://lists.xiph.org/pipermail/flac-dev/attachments/20150906/462771c9/attachment.obj
lvqcl wrote:> The patch http://git.xiph.org/?p=flac.git;a=commitdiff;h=0bea5fb96436588b4e78c5be79bf174b9be7a202 > changes the type of t value from FLAC__int32 to uint32_t, > but only for 24-bit signed input samples. 24-bit *un*signed > input still uses FLAC__int32 t.These UB fixes were driven purely by UBSan warnings generated while running the test suite. The fact that no warning was gneerated by 24 bit unsigned input suggests that the test suite doesn't cover 24 bit unsigned input. That's probably something that should be fixed.> So I wonder - does it makes sense to change this type too? > The patch is attached.That looks ok, but a test is needed first. I'll see if I can come up with something. Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/
Erik de Castro Lopo wrote:> These UB fixes were driven purely by UBSan warnings generated while running > the test suite. The fact that no warning was gneerated by 24 bit unsigned > input suggests that the test suite doesn't cover 24 bit unsigned input.Yes, I can see only --sign=signed option in various test scripts.
lvqcl wrote:> So I wonder - does it makes sense to change this type too? > The patch is attached.I updated the tests to test unsigned as well as signed and then applied your patch. Cheers, Erik -- ---------------------------------------------------------------------- Erik de Castro Lopo http://www.mega-nerd.com/