On Fri Dec 15 13:50:25 2023 +0000, Giovanni Mascellani wrote:
Well, we've definitely had Wine bugs filed for assertions in the HLSL
compiler triggered by trying to compile a particular shader. What would have been the alternative? Maybe everything magically works anyway by luck, maybe you just get a bad output, maybe the bug resurfaces in a totally different place and you have to spend a day of work to understand what it is, instead of having an assertion message that immediately tells you which invariant has been violated. And still, if you're willing to bet on the first two alternatives, you can use `-DNDEBUG` and have it. But you can't do the other way around. So I think it's better to leave the opportunity on the table. As you say, it's true that a user using their distribution's packages doesn't directly control `-DNDEBUG`, but first they can recompile packages, if they deem it useful or necessary; and second the user is already trusting the distribution with a lot of decisions on how their operating system is supposed to work: this is just another one, and each distribution can have its own philosophy, and each user can choose which distribution better matches their own philosophy. Again, by leaving assertions in the code you're leaving more freedom on the table, not less. If you think that vkd3d should by default use `-DNDEBUG`, that's a sensible point (but I'll propose to have assertions enabled in the CI). That's what Conor is proposing, basically.
I don't think that's a decision a library can reliably make.
If the library's internal state is inconsistent, it basically cannot reliably make any decision any more. So it's not a bad idea to make this condition apparent.
In theory, sure. In practice, I think we have a number of cases where
assert() is essentially used as a substitute for input validation. It also tends to make review a bit harder; instead of verifying the condition is handled appropriately, the reviewer needs to verify it can never happen. Yeah, `assert()` should not be used for input validation, I totally agree (and while it might seem that I at some point I wanted to use it for input validation in the context of the VSIR validator, let me point out that I didn't mean the VSIR validator to check user input, I was convinced that the TPF parser would already do input validation, the principle still stands). Also, the reviewer probably has to check the asserted condition anyway. The assertion only makes it clearer what condition has to be checked and helps the reviewer ensuring that in practice the condition is not violated during test runs, so I think it is helpful. To elaborate, suppose that I have this code:
assert(buffer.size >= 3); do_something(buffer.buffer[0]); do_something(buffer.buffer[1]); do_something(buffer.buffer[2]);If we remove the `assert()`, the reviewer still has to ensure that the buffer is big enough.
Having assertions or assertion-like checks which are excluded from releases, but included in our test builds, definitely would improve the odds of catching errors in new code, and regressions. Probably a very small subset, but that's not nothing.