Re: [PATCH v1 01/15] net: devmem: pull struct definitions out of ifdef

[Pavel Begunkov <asml.silence@gmail.com>]

On 10/10/24 19:01, Mina Almasry wrote:
> On Wed, Oct 9, 2024 at 4:16 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 10/9/24 21:17, Mina Almasry wrote:
>>> On Mon, Oct 7, 2024 at 3:16 PM David Wei <dw@davidwei.uk> wrote:
>>>>
>>>> From: Pavel Begunkov <asml.silence@gmail.com>
>>>>
>>>> Don't hide structure definitions under conditional compilation, it only
>>>> makes messier and harder to maintain. Move struct
>>>> dmabuf_genpool_chunk_owner definition out of CONFIG_NET_DEVMEM ifdef
>>>> together with a bunch of trivial inlined helpers using the structure.
>>>>
>>>
>>> To be honest I think the way it is is better? Having the struct
>>> defined but always not set (because the code to set it is always
>>> compiled out) seem worse to me.
>>>
>>> Is there a strong reason to have this? Otherwise maybe drop this?
>> I can drop it if there are strong opinions on that, but I'm
>> allergic to ifdef hell and just trying to help to avoid it becoming
>> so. I even believe it's considered a bad pattern (is it?).
>>
>> As for a more technical description "why", it reduces the line count
>> and you don't need to duplicate functions. It's always annoying
>> making sure the prototypes stay same, but this way it's always
>> compiled and syntactically checked. And when refactoring anything
>> like the next patch does, you only need to change one function
>> but not both. Do you find that convincing?
>>
> 
> To be honest the tradeoff wins in the other direction for me. The
> extra boiler plate is not that bad, and we can be sure that any code

We can count how often people break builds because a change
was compiled just with one configuration in mind. Unfortunately,
I did it myself a fair share of times, and there is enough of
build robot reports like that. It's not just about boiler plate
but rather overall maintainability.

> that touches net_devmem_dmabuf_binding will get a valid internals
> since it won't compile if the feature is disabled. This could be
> critical and could be preventing bugs.

I don't see the concern, if devmem is compiled out there wouldn't
be a devmem provider to even create it, and you don't need to
worry. If you think someone would create a binding without a devmem,
then I don't believe it'd be enough to hide a struct definition
to prevent that in the first place.

I think the maintainers can tell whichever way they think is
better, I can drop the patch, even though I think it's much
better with it.

-- 
Pavel Begunkov