Although I might disagree with the style of the write up, I think it is right that there are weak engineering practices applied:
- not enough information provided in commit message.
- this change ought to have been split into smaller chunks (e.g. introduce indirection whilst code continues to use hardware)
- limited (automated) testing
- potential confusion by the author over C concepts
- additional complexity which could have been hidden behind a cleaner interface rather than using defines
I agree about your points about:
- defines over multiple repo's made this more complicated that was necessary.
- fault injections would have been the appropriate way to test this (or maybe mocks)
- testing of randomness needs careful design, e.g. RNG seeding can be a good way
Taking a step back, we as a community of excellence need to emphasise that this isn't a criticism about the person/author. There needs to be a clear analysis of what went wrong whilst being kind to the person/people involved.
- not enough information provided in commit message. - this change ought to have been split into smaller chunks (e.g. introduce indirection whilst code continues to use hardware) - limited (automated) testing - potential confusion by the author over C concepts - additional complexity which could have been hidden behind a cleaner interface rather than using defines
I agree about your points about:
- defines over multiple repo's made this more complicated that was necessary. - fault injections would have been the appropriate way to test this (or maybe mocks) - testing of randomness needs careful design, e.g. RNG seeding can be a good way
Taking a step back, we as a community of excellence need to emphasise that this isn't a criticism about the person/author. There needs to be a clear analysis of what went wrong whilst being kind to the person/people involved.