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
