Re: [PATCH 3/3] ASoC: SDCA: functions: Fix confusing cleanup.h syntax

[Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>]

On 04/12/2025 12:26, Charles Keepax wrote:
> On Wed, Dec 03, 2025 at 05:12:40PM +0100, Krzysztof Kozlowski wrote:
>> Initializing automatic __free variables to NULL without need (e.g.
>> branches with different allocations), followed by actual allocation is
>> in contrary to explicit coding rules guiding cleanup.h:
>>
>> "Given that the "__free(...) = NULL" pattern for variables defined at
>> the top of the function poses this potential interdependency problem the
>> recommendation is to always define and assign variables in one statement
>> and not group variable definitions at the top of the function when
>> __free() is used."
>>
>> Code does not have a bug, but is less readable and uses discouraged
>> coding practice, so fix that by moving declaration to the place of
>> assignment.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
>> ---
> 
> Hmm... yeah a fair point. Can't say I love the inline
> declarations but I guess I will get used to it.
> 
> I wonder if for consistency we should do something about
> the __free in find_sdca_init_table as well. One could move
> the alloc to before the error checks, does risk doing an
> unecessary alloc but its on the error path so I don't feel like
> performance matters. Or rather than sizeof(*raw) we could just
> do sizeof(struct raw_init_write)?

If there is a reason to deviate from the declaration+allocation rule,
then sure go for it. This is style, so it describes only the preference.
But if you do not have a reason, then coding style described in
cleanup.h should be used (or don't use cleanup.h).

> 
> But if you would rather leave as is I don't mind either so:

Yep. Few sdca-related files use that exception. The
find_sdca_init_table() at least has a reason - it uses sizeof(*raw) as
you mentioned, so that's why I decided not to change it. Just too much
churn.

But I want to fix all the cases where '= NULL' is used without any need.


> 
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> Thanks,
> Charles


Best regards,
Krzysztof