On 19/08/2021 14.32, Wolfgang Denk wrote:
> The existence of bad code is not a justification to add more of it.
Obviously true and I agree.
However, it is at the same time completely irrelevant in this context,
because the pattern of using the return value of dev_get_priv() without
a NULL check is neither bad or wrong, as has now been explained to you
several times.
If you really think checking the return value of dev_get_priv() must be
done religiously, perhaps you could tap Stefan (737c3de09984), Marek
(7e1f1e16fe75), or Heiko (6e31c62a175c) on the shoulder and tell them to
stop cranking out "bad" code.
On 19/08/2021 16.16, Wolfgang Denk wrote:
> I mean, look at the implementation of dev_get_priv():
>
> 628 void *dev_get_priv(const struct udevice *dev)
> 629 {
> 630 if (!dev) {
> 631 dm_warn("%s: null device\n", __func__);
> 632 return NULL;
> 633 }
> 634
> 635 return dm_priv_to_rw(dev->priv_);
> 636 }
>
> If there is guaranteed no way that dev_get_priv() can return a NULL
> pointer, that means that it must be guaranteed that the "dev"
> argument can never be a NULL pointer, either.
There's another logical fallacy right here. Sure, you've found an input
value for which dev_get_priv() would return NULL. But any caller who
knows they're not passing a NULL dev also know they won't follow that
code path.
A driver which doesn't populate the priv field by via a non-zero
.priv_auto field may need to check the return value of dev_get_priv().
I'm not claiming that checking that is always redundant. However,
neither is it anywhere near true that checking is always required.
Rasmus
