Re: [PATCH 02/03] input: bitmap update for sh_keysc

[Paul Mundt <lethal@linux-sh.org>]

On Mon, Feb 08, 2010 at 04:41:16PM +0900, Magnus Damm wrote:
> Hi Dmitry,
> 
> On Mon, Feb 8, 2010 at 4:18 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Feb 08, 2010 at 03:32:34PM +0900, Magnus Damm wrote:
> >>
> >> +#define WRAP(fn, m...) bitmap_##fn(m, SH_KEYSC_MAXKEYS)
> >> +#define sh_keysc_map_zero(m) WRAP(zero, (m)->b)
> >> +#define sh_keysc_map_fill(m) WRAP(fill, (m)->b)
> >> +#define sh_keysc_map_and(m, m2) WRAP(and, (m)->b, (m)->b, (m2)->b)
> >> +#define sh_keysc_map_or(m, m2) WRAP(or, (m)->b, (m)->b, (m2)->b)
> >> +#define sh_keysc_map_complement(m) WRAP(complement, (m)->b, (m)->b)
> >> +#define sh_keysc_map_set(m, n) set_bit((n), (m)->b)
> >> +#define sh_keysc_map_clear(m, n) clear_bit((n), (m)->b)
> >> +#define sh_keysc_map_test(m, n) test_bit((n), (m)->b)
> >> +
> >
> > Why do you need these wrappers? For me they simply create a distraction,
> > later when I read the code I will have to go and look up what
> > sh_keysc_map_set() means but if I see __set_bit() I'd know right away.
> 
> To avoid duplicating SH_KEYSC_MAXKEYS all over the place I started out
> by wrapping bitmap_zero/fill/and/or/complement(). To be consistent I
> decided to wrap the set/clear/test_bit() functions as well, but it may
> be better to leave them alone.
> 
> Are you ok with wrapping the bitmap_...() functions to avoid
> duplicating SH_KEYSC_MAXKEYS?
> 
I don't see the point, either. The bitmap routines take the maximum size
of the bitmap as an argument, and so code that uses the API heavily and
has the size defined in a macro is going to have it sprinkled around, who
cares?

Wrapping up the bitmap API in your own wrappers to hide this fact only
makes it more confusing and really doesn't buy you anything. While things
like sh_keysc_map_set/clear() might seem obvious enough, it's also
impossible to tell whether these employ __set_bit()/__clear_bit() or
set_bit()/clear_bit() semantics without having to scroll back and look
them up. The added obfuscation to save yourself from having to sprinkle
SH_KEYSC_MAXKEYS around simply isn't worth it.

Any time you start out wrapping an existing API for private driver use
outside of things like I/O accessors, you're almost certainly going to
run in to trouble.