+ } while (0)
static const struct input_device_id *input_match_device(struct
input_handler *handler,
struct input_dev
*dev)
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This changes semantics. While current implementation of MATCH_BIT might
not be great style, it is not supposed to be used outside of
input_match_device(), and the replacement is completely broken.
Thanks.
--
Dmitry
this patch definitely changes semantics.
so please discard it, sorry about that!
On Wed, Jul 25, 2012 at 1:37 PM, Dmitry Torokhov
[off-list ref] wrote:
On Wed, Jul 25, 2012 at 01:20:56PM +0800, Baodong Chen wrote:
This changes semantics. While current implementation of MATCH_BIT might
not be great style, it is not supposed to be used outside of
input_match_device(), and the replacement is completely broken.
Thanks.
--
Dmitry
This changes semantics. While current implementation of MATCH_BIT might
not be great style, it is not supposed to be used outside of
input_match_device(), and the replacement is completely broken.
Replacement is certainly completely broken, but "might not be great style"
is quite an understatement. Everything else aside, it's too obfuscated
and ugly for anything outside of IOCCC and too verbose for IOCCC.
Damnit, at least turn that into
static inline int is_subset(unsigned long *bitmap1, unsigned long *bitmap2, size_t bits)
{
int i;
for (i = BITS_TO_LONGS(bits); i; bitmap1++, bitmap2++, i--)
if ((*bitmap1 & *bitmap2) != *bitmap1)
return 0;
return 1;
}
and just before your loop
/* ignore ones that don't have bits required by id */
#define MATCH_BIT(array, bits) if (!is_subset(id->array, dev->array, bits)) continue;
with #undef MATCH_BIT right after the loop, to make it damn clean that it's local.
And frankly, I'd consider expanding that variant and killing the macro off.
Macros that affect control flow are vile and actively confusing for reader.
This changes semantics. While current implementation of MATCH_BIT might
not be great style, it is not supposed to be used outside of
input_match_device(), and the replacement is completely broken.
Replacement is certainly completely broken, but "might not be great style"
is quite an understatement. Everything else aside, it's too obfuscated
and ugly for anything outside of IOCCC and too verbose for IOCCC.
Damnit, at least turn that into
static inline int is_subset(unsigned long *bitmap1, unsigned long *bitmap2, size_t bits)
{
int i;
for (i = BITS_TO_LONGS(bits); i; bitmap1++, bitmap2++, i--)
if ((*bitmap1 & *bitmap2) != *bitmap1)
return 0;
return 1;
}
and just before your loop
/* ignore ones that don't have bits required by id */
#define MATCH_BIT(array, bits) if (!is_subset(id->array, dev->array, bits)) continue;
Hmm, we already have bitmap_subset(), I think we can simply use it.
with #undef MATCH_BIT right after the loop, to make it damn clean that it's local.
And frankly, I'd consider expanding that variant and killing the macro off.
Macros that affect control flow are vile and actively confusing for reader.
I agree. In my defense it is quite old code...
--
Dmitry
This changes semantics. While current implementation of MATCH_BIT might
not be great style, it is not supposed to be used outside of
input_match_device(), and the replacement is completely broken.
Replacement is certainly completely broken, but "might not be great style"
is quite an understatement. Everything else aside, it's too obfuscated
and ugly for anything outside of IOCCC and too verbose for IOCCC.
Damnit, at least turn that into
static inline int is_subset(unsigned long *bitmap1, unsigned long *bitmap2, size_t bits)
{
int i;
for (i = BITS_TO_LONGS(bits); i; bitmap1++, bitmap2++, i--)
if ((*bitmap1 & *bitmap2) != *bitmap1)
return 0;
return 1;
}
and just before your loop
/* ignore ones that don't have bits required by id */
#define MATCH_BIT(array, bits) if (!is_subset(id->array, dev->array, bits)) continue;
Hmm, we already have bitmap_subset(), I think we can simply use it.
quoted
with #undef MATCH_BIT right after the loop, to make it damn clean that it's local.
And frankly, I'd consider expanding that variant and killing the macro off.
Macros that affect control flow are vile and actively confusing for reader.
I agree. In my defense it is quite old code...
So how about the patch below?
--
Dmitry
Input: get rid of MATCH_BIT() macro
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
MATCH_BIT() is ugly and stupid, we have much nicer bitmap_subset() which
does the same and does not hide control flow.
Reported-by: Baodong Chen <redacted>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/input.c | 43 ++++++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 17 deletions(-)