Re: [PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()

Marek Vasut
2024-03-26T22:20:07Z

On 3/20/24 10:00 PM, Laurent Pinchart wrote:
> On Wed, Mar 20, 2024 at 09:52:34PM +0100, Marek Vasut wrote:
>> On 3/18/24 5:18 PM, Laurent Pinchart wrote:
>>
>>>> @@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
>>>>
>>>> s = env_get("bootm_low");
>>>> if (s)
>>>> - tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
>>>> + tmp = simple_strtoull(s, NULL, 16);
>>>> else
>>>> tmp = start;
>>>>
>>>
>>> Maybe you could even drop the tmp variable completely by writing this
>>>
>>> if (s)
>>> size -= simple_strtoull(s, NULL, 16) - start;
>>>
>>> return size;
>>>
>>> I've never liked variables named tmp :-)
>>
>> No, let's not do this. With this FDT part, the code should be verbose
>> and as easy to understand at first glance as possible, no subtraction
>> assignments and other shenanigans please.
>
> How about this ?
>
> s = env_get("bootm_low");
> if (s) {
> phys_addr_t low_addr = simple_strtoull(s, NULL, 16);
> size -= low_addr - start;
> }
>
> return size;
>
> If you're going for readability, that's clearer than
>
> return size - (tmp - start);

I do not share this opinion, the subtraction assignment makes this
harder to read. If that makes for a more verbose code, so be it.

> and it also interestingly points out that tmp/low_addr should be a
> phys_addr_t, not a phys_size_t.

The original code already contains trivial 'tmp = start' assignment, so
these two types should match, and they don't. Fixed in V4, thanks.