search

mackron / dr_libs

Audio decoding libraries for C/C++, each in a single source file.
Other
1.27k stars 206 forks source link

dr_flac: A part of conditional expression is always true: blockSize >= 2. #117

Closed kcgen closed 4 years ago

kcgen commented 4 years ago

image

https://www.viva64.com/en/w/v560/

mackron commented 4 years ago

Will not fix. I prefer the readability of an explicit check in this case. This also maps better with how it's specified in the FLAC spec.

kcgen commented 4 years ago

Thanks for the assessment on this. Regarding this theme; would an assert make sense instead? (I suppose if the values in the assert are derived from user content, then they should stay runtime checks even in release builds; but if the values are internal-logic or states, then an assert might be acceptable).

mackron commented 4 years ago

An assert would work, but it's not about that - it's about the readability and maintainability of the code. This is a technical part of the code, and I think the run time logic should match the FLAC spec as closely as possible to ease maintainability in case I need to back and cross reference with the spec (they specify an explicit range, just like I have it in code). I also just don't want to put an assert in that particular part of the code because it looks ugly. I did, however, add an assert at the top of that block to check the blockSize > 0 case (won't silence this warning, though).

kcgen commented 4 years ago

Thanks for the reasoning, which makes sense. Also agree that removing this check would make the code obvious. Adding to ignore list!

On Tue., Jan. 28, 2020, 3:39 a.m. David Reid, notifications@github.com wrote:

An assert would work, but it's not about that - it's about the readability and maintainability of the code. This is a technical part of the code, and I think the run time logic should match the FLAC spec as closely as possible to ease maintainability in case I need to back and cross reference with the spec (they specify an explicit range, just like I have it in code). I also just don't want to put an assert in that particular part of the code because it looks ugly. I did, however, add an assert at the top of that block to check the blockSize > 0 case (won't silence this warning, though).