Mir

Code review comment for lp:~vanvugt/mir/saucy-build

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> 58 +#ifdef NDEBUG
>
> Instead of disabling the test I would propose replacing the assert() in the

This is something I debated with Thomas when he added test like this.

/1/ A test represents a requirement on the behavior of code.

/2/ A precondition is something that code is allowed to assume (as it is the responsibility of the caller to ensure it holds).

/3/ The code has no responsibility to behave in a particular way if preconditions are violated. Its behavior is unspecified (or even undefined).

/4/ Tests that "preconditions violations" are reported correctly imply that they are not really preconditions - just conditions.

The point: *if* we want some preconditions to be conditions in debug builds, then it makes sense for the corresponding tests to be conditionally compiled.

Personally, I don't think these checks should ever be conditions *with tests to validate the behavior*. But unless there are performance reasons I feel that is is as important to aid problem diagnosis in release builds as in debug ones.

review: Approve

« Back to merge proposal