Hi Edward,
On 09.09.22 04:15, Edward O'Callaghan via flashrom wrote:
While refactoring flashrom.c to move towards a reentrant flashrom a pragmatic choice of utilising NULLity to represent the default state of a struct at instiation was used in the following and related commits - https://review.coreboot.org/c/flashrom/+/67391/
I had some trouble parsing this. I've never seen the term nullity in a programming context. I first assumed what is meant is that we use the fact that struct fields unmentioned in an initialization are 0 (which works as NULL for pointers). But later nothing made sense, so maybe: do you mean the 'meaning' (or interpretation of struct contents) when you write 'state'?
Basically using NULL as the default state rather than an invalid state allows for memory address by value to be in a boolean state of either 'defined to be default' or 'defined to be custom' - over that of a tristate of 'defiled to be default explicitly', 'defined to be custom' or NULL which must be manually validated as 'invalid'. This is quite pragmatic as we can rid ourselves of explicit nullarity checks at runtime and instead have always defined reasonable defaults at compile-time thus making functions domains more well defined at compile-time. It is also pragmatic and arguably more modern from the stand-point of requiring less code to effectively obtain a valid value from the instantiated type not unlike Rust's Default trait or Ada's ability to have default values in a Record and so on.
TL;DR I agree it seems modern. But implicit behavior can also make it harder to understand code, especially for new people. Also, every case can be different.
There is always the question how it looks to the reader, i.e. will they know that there is a default action or will they wrongfully assume that NULL means there will be no action carried out. In case of the `delay` action, I think we are safe. That we need to delay is not an aspect of the programmer driver; it can only decide how we delay. So that there is always a (default) delay seems clear.
Other cases might be less clear. And there are even cases where explicit NULL checks should still be done, for instance when two custom implemen- tations are mutually exclusive. And now the meanest example of all: Our default_spi_command() and default_spi_multicommand(). It's ok to have custom implementations for each of them and also for both of them. But we need at least one of them. So rules may be more complex than 'NULL is ok' / 'NULL is not ok', and then checks can't be avoided even if we use NULL-means-default.
There is another aspect that I don't feel strong about. IMO, it is more important to educate each other to avoid NULL in the first place than to check for it everywhere. There are a few cases where it makes sense to check, though. For instance it is easy to miss a field in our structs full of function pointers (e.g. `flashchip` `*_master`). Now, if we pro- vide a (default) meaning to NULL in some cases, that might trick inex- perienced developers into thinking that NULL is generally allowed.
Cheers, Nico