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-le...@flashrom.org
_______________________________________________
flashrom mailing list -- flashrom@flashrom.org
To unsubscribe send an email to flashrom-le...@flashrom.org
