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

[Mina Almasry <almasrymina@google.com>]

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
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.

-- 
Thanks,
Mina