Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac functions
On Sun, Dec 30, 2018 at 11:12 PM Finn Thain wrote:
> On Sun, 30 Dec 2018, LEROY Christophe wrote:
> 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".
Ok, I failed to realized that MAC and ATARI are not mutually exclusive.
I agree that your original version is best then.
Arnd
Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac functions
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
>
Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac functions
Finn Thain a écrit :
On Sat, 29 Dec 2018, Arnd Bergmann wrote:
On Wed, Dec 26, 2018 at 1:43 AM Finn Thain
wrote:
> +
> +static ssize_t m68k_nvram_get_size(void)
> +{
> + if (MACH_IS_ATARI)
> + return atari_nvram_get_size();
> + else if (MACH_IS_MAC)
> + return mac_pram_get_size();
> + return -ENODEV;
> +}
> +
> +/* Atari device drivers call .read (to get checksum validation) whereas
> + * Mac and PowerMac device drivers just use .read_byte.
> + */
> +const struct nvram_ops arch_nvram_ops = {
> +#ifdef CONFIG_MAC
> + .read_byte = m68k_nvram_read_byte,
> + .write_byte = m68k_nvram_write_byte,
> +#endif
> +#ifdef CONFIG_ATARI
> + .read = m68k_nvram_read,
> + .write = m68k_nvram_write,
> + .set_checksum = m68k_nvram_set_checksum,
> + .initialize = m68k_nvram_initialize,
> +#endif
> + .get_size = m68k_nvram_get_size,
> +};
> +EXPORT_SYMBOL(arch_nvram_ops);
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 ?
I don't see potential code duplication here, do you ?
Christophe
I was actually expecting one more patch here that would make the
arch_nvram_ops a pointer to one of multiple structures, which would
be easier to do with multiple copies, but I suppose there is no need
for that here (there might be on ppc, I have to look again).
Yes, I considered that too. I picked the variation that makes everything
const.
--
Arnd
Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac functions
On Sat, 29 Dec 2018, Arnd Bergmann wrote:
> On Wed, Dec 26, 2018 at 1:43 AM Finn Thain wrote:
>
> > +
> > +static ssize_t m68k_nvram_get_size(void)
> > +{
> > + if (MACH_IS_ATARI)
> > + return atari_nvram_get_size();
> > + else if (MACH_IS_MAC)
> > + return mac_pram_get_size();
> > + return -ENODEV;
> > +}
> > +
> > +/* Atari device drivers call .read (to get checksum validation) whereas
> > + * Mac and PowerMac device drivers just use .read_byte.
> > + */
> > +const struct nvram_ops arch_nvram_ops = {
> > +#ifdef CONFIG_MAC
> > + .read_byte = m68k_nvram_read_byte,
> > + .write_byte = m68k_nvram_write_byte,
> > +#endif
> > +#ifdef CONFIG_ATARI
> > + .read = m68k_nvram_read,
> > + .write = m68k_nvram_write,
> > + .set_checksum = m68k_nvram_set_checksum,
> > + .initialize = m68k_nvram_initialize,
> > +#endif
> > + .get_size = m68k_nvram_get_size,
> > +};
> > +EXPORT_SYMBOL(arch_nvram_ops);
>
> 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 was actually expecting one more patch here that would make the
> arch_nvram_ops a pointer to one of multiple structures, which would
> be easier to do with multiple copies, but I suppose there is no need
> for that here (there might be on ppc, I have to look again).
>
Yes, I considered that too. I picked the variation that makes everything
const.
--
>Arnd
>
Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac functions
On Wed, Dec 26, 2018 at 1:43 AM Finn Thain wrote:
> +
> +static ssize_t m68k_nvram_get_size(void)
> +{
> + if (MACH_IS_ATARI)
> + return atari_nvram_get_size();
> + else if (MACH_IS_MAC)
> + return mac_pram_get_size();
> + return -ENODEV;
> +}
> +
> +/* Atari device drivers call .read (to get checksum validation) whereas
> + * Mac and PowerMac device drivers just use .read_byte.
> + */
> +const struct nvram_ops arch_nvram_ops = {
> +#ifdef CONFIG_MAC
> + .read_byte = m68k_nvram_read_byte,
> + .write_byte = m68k_nvram_write_byte,
> +#endif
> +#ifdef CONFIG_ATARI
> + .read = m68k_nvram_read,
> + .write = m68k_nvram_write,
> + .set_checksum = m68k_nvram_set_checksum,
> + .initialize = m68k_nvram_initialize,
> +#endif
> + .get_size = m68k_nvram_get_size,
> +};
> +EXPORT_SYMBOL(arch_nvram_ops);
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?
I was actually expecting one more patch here that would make the
arch_nvram_ops a pointer to one of multiple structures, which would
be easier to do with multiple copies, but I suppose there is no need
for that here (there might be on ppc, I have to look again).
Arnd
