Hello,
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/
It was brought to my attention that this is perhaps considered a more substantive change to the general practice within flashrom and so I just wanted to give a brief prelude to the rationale and if anyone has any reason why they don't like it.
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.
While not particularly novel I did want to follow up on the request to highlight it here.
Kindly, Edward.
Hi Edward, thanks for bringing concepts of modern C to flashrom. +1 for using NULL and then falling back to a default implementation in the logic. IMO it makes the code much better.
-- Thomas
On Fri, 2022-09-09 at 12:15 +1000, Edward O'Callaghan via flashrom wrote:
Hello,
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/
It was brought to my attention that this is perhaps considered a more substantive change to the general practice within flashrom and so I just wanted to give a brief prelude to the rationale and if anyone has any reason why they don't like it.
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.
While not particularly novel I did want to follow up on the request to highlight it here.
Kindly, Edward. _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
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
On Fri, 2022-09-09 at 15:35 +0200, Nico Huber wrote:
Hi Edward,
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.
When combining some changes, such as combining spi_master.command() and spi_master.multicommand(), renaming spi_master.read() to spi_master.optimized_read() and allowing spi_master.optimized_read() to be NULL, there will be good improvement in understandability. The logic is expressed through direct branching (using if) instead of jumping along a concatination of function pointers. The simplefied callgraphs below can give a hint on how it may look then.
// The current implementation // mst->spi.read = default_spi_read() // mst->spi.command = default_spi_send_command spi.c: spi_chip_read() programmer_impl: mst->spi.read spi.c: default_spi_read() ... spi.c: spi_send_command() programmer_impl: mst->spi.command() spi.c: default_spi_send_command() spi.c: spi_send_multicommand() programmer_impl: mst->spi.multicommand()
// Possible future implementation with the changes // mst->spi.commands replaces mst->spi.command & mst->spi.multicommand // mst->spi.optimized_read replaces mst->spi.read // mst->spi.optimized_read = NULL (default NULL) spi.c: spi_chip_read() if (mst->spi.optimized_read) mst->spi.optimized_read() else spi_read_via_command() spi.c: spi_read_via_command() programmer_impl: mst->spi.commands()
So using default NULL alone may be not good, but when combing it with other changes it can give a good improvement over the current way the code is working.
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.
IMO combining those too into one spi_master function pointer may bring a loop more into the programmer, but frees up complexity at the other end. Furthermore we can get in a state that an entry in the spi_master struct or other structs are either mandetory or optional. So having no mutally exclusions. Then the meaning of NULL is also clear.
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.
I can't agree with you. In a modern interpretation of C, 0 / NULL can be used in a well defined ways, especially as default value in structs and it is nothing to avoid. I would educate people how to use NULL in a good way, rather than avoiding it.
Cheers, Nico
-- Thomas
flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
-
Edward O'Callaghan -
Nico Huber -
Thomas Heijligen