Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac functions

[Finn Thain]

On Sun, 30 Dec 2018, LEROY Christophe wrote:

> > > 
> > > Since the operations are almost entirely distinct, why not have two 
> > > separate 'nvram_ops' instances here that each refer to just the set 
> > > they actually need?
> > > 
> > 
> > The reason for that is that I am alergic to code duplication. But I'll 
> > change it if you think it matters. BTW, this patch has already been 
> > acked by Geert.
> 
> I agree it would be cleaner, as it would also avoid this 
> m68k_nvram_get_size() wouldn't it ?
> 

No, that function makes run-time decisions. #ifdef won't work.

> I don't see potential code duplication here, do you ?
> 

Here's my problem with Arnd's suggestion. Consider this C code,

#ifdef FOO
const struct nvram_ops arch_nvram_ops = {
	/* ... */
}
#else
const struct nvram_ops arch_nvram_ops = {
        /* ... */
}
#endif

Lets say you write a hypothetical patch to remove the 'const'. Now you 
have two 'const' keywords to edit, and you have the risk of overlooking 
one of them. The solution to this problem is sometimes referred to as 
"DRY", meaning Don't Repeat Yourself:

const struct nvram_ops arch_nvram_ops = {
        /* ... */
#ifdef FOO
        /* ... */
#else
        /* ... */
#endif
        /* ... */
}

But I'm over-simplifying. Arnd's alternative actually goes like this,

#if defined(CONFIG_MAC) && !defined(CONFIG_ATARI)
const struct nvram_ops arch_nvram_ops = {
        /* ... */
}
#elif !defined(CONFIG_MAC) && defined(CONFIG_ATARI)
const struct nvram_ops arch_nvram_ops = {
        /* ... */
}
#elif defined(CONFIG_MAC) && defined(CONFIG_ATARI)
const struct nvram_ops arch_nvram_ops = {
        /* ... */
}
#endif

So, you're right, this isn't "duplication", it's "triplication".

-- 

> Christophe
>