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