On Wed, Oct 9, 2024 at 4:16 PM Pavel Begunkov <asml.silence@xxxxxxxxx> wrote: > > On 10/9/24 21:17, Mina Almasry wrote: > > On Mon, Oct 7, 2024 at 3:16 PM David Wei <dw@xxxxxxxxxxx> wrote: > >> > >> From: Pavel Begunkov <asml.silence@xxxxxxxxx> > >> > >> 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
