* [PATCH] fixed a macro coding style issue [not found] <no> @ 2012-07-25 5:20 ` Baodong Chen 2012-07-25 5:27 ` Venu Byravarasu 2012-07-25 5:37 ` Dmitry Torokhov 0 siblings, 2 replies; 7+ messages in thread From: Baodong Chen @ 2012-07-25 5:20 UTC (permalink / raw) To: dmitry.torokhov; +Cc: linux-input, linux-kernel, Baodong Chen Fixed a coding style issue in driver/input/input.c Signed-off-by: Baodong Chen <chenbdchenbd@gmail.com> --- drivers/input/input.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/input/input.c b/drivers/input/input.c index 8921c61..c96e983 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -845,11 +845,13 @@ int input_set_keycode(struct input_dev *dev, EXPORT_SYMBOL(input_set_keycode); #define MATCH_BIT(bit, max) \ + do { \ for (i = 0; i < BITS_TO_LONGS(max); i++) \ if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \ break; \ - if (i != BITS_TO_LONGS(max)) \ - continue; + if (i != BITS_TO_LONGS(max)) \ + continue; \ + } while (0) static const struct input_device_id *input_match_device(struct input_handler *handler, struct input_dev *dev) -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH] fixed a macro coding style issue 2012-07-25 5:20 ` [PATCH] fixed a macro coding style issue Baodong Chen @ 2012-07-25 5:27 ` Venu Byravarasu 2012-07-25 5:37 ` Dmitry Torokhov 1 sibling, 0 replies; 7+ messages in thread From: Venu Byravarasu @ 2012-07-25 5:27 UTC (permalink / raw) To: Baodong Chen, dmitry.torokhov@gmail.com Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org > -----Original Message----- > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > owner@vger.kernel.org] On Behalf Of Baodong Chen > Sent: Wednesday, July 25, 2012 10:51 AM > To: dmitry.torokhov@gmail.com > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Baodong > Chen > Subject: [PATCH] fixed a macro coding style issue > > Fixed a coding style issue in driver/input/input.c > > Signed-off-by: Baodong Chen <chenbdchenbd@gmail.com> > --- > drivers/input/input.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/input.c b/drivers/input/input.c > index 8921c61..c96e983 100644 > --- a/drivers/input/input.c > +++ b/drivers/input/input.c > @@ -845,11 +845,13 @@ int input_set_keycode(struct input_dev *dev, > EXPORT_SYMBOL(input_set_keycode); > > #define MATCH_BIT(bit, max) \ > + do { \ > for (i = 0; i < BITS_TO_LONGS(max); i++) \ > if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \ > break; \ > - if (i != BITS_TO_LONGS(max)) \ > - continue; > + if (i != BITS_TO_LONGS(max)) \ > + continue; \ What is the need for extra indentation here? > + } 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/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixed a macro coding style issue 2012-07-25 5:20 ` [PATCH] fixed a macro coding style issue Baodong Chen 2012-07-25 5:27 ` Venu Byravarasu @ 2012-07-25 5:37 ` Dmitry Torokhov 2012-07-25 6:09 ` Baodong Chen 2012-07-25 6:15 ` Al Viro 1 sibling, 2 replies; 7+ messages in thread From: Dmitry Torokhov @ 2012-07-25 5:37 UTC (permalink / raw) To: Baodong Chen; +Cc: linux-input, linux-kernel On Wed, Jul 25, 2012 at 01:20:56PM +0800, Baodong Chen wrote: > Fixed a coding style issue in driver/input/input.c > > Signed-off-by: Baodong Chen <chenbdchenbd@gmail.com> > --- > drivers/input/input.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/input.c b/drivers/input/input.c > index 8921c61..c96e983 100644 > --- a/drivers/input/input.c > +++ b/drivers/input/input.c > @@ -845,11 +845,13 @@ int input_set_keycode(struct input_dev *dev, > EXPORT_SYMBOL(input_set_keycode); > > #define MATCH_BIT(bit, max) \ > + do { \ > for (i = 0; i < BITS_TO_LONGS(max); i++) \ > if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \ > break; \ > - if (i != BITS_TO_LONGS(max)) \ > - continue; > + if (i != BITS_TO_LONGS(max)) \ > + continue; \ > + } while (0) 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixed a macro coding style issue 2012-07-25 5:37 ` Dmitry Torokhov @ 2012-07-25 6:09 ` Baodong Chen 2012-07-25 6:15 ` Al Viro 1 sibling, 0 replies; 7+ messages in thread From: Baodong Chen @ 2012-07-25 6:09 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, linux-kernel this patch definitely changes semantics. so please discard it, sorry about that! On Wed, Jul 25, 2012 at 1:37 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Jul 25, 2012 at 01:20:56PM +0800, Baodong Chen wrote: >> Fixed a coding style issue in driver/input/input.c >> >> Signed-off-by: Baodong Chen <chenbdchenbd@gmail.com> >> --- >> drivers/input/input.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/input/input.c b/drivers/input/input.c >> index 8921c61..c96e983 100644 >> --- a/drivers/input/input.c >> +++ b/drivers/input/input.c >> @@ -845,11 +845,13 @@ int input_set_keycode(struct input_dev *dev, >> EXPORT_SYMBOL(input_set_keycode); >> >> #define MATCH_BIT(bit, max) \ >> + do { \ >> for (i = 0; i < BITS_TO_LONGS(max); i++) \ >> if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \ >> break; \ >> - if (i != BITS_TO_LONGS(max)) \ >> - continue; >> + if (i != BITS_TO_LONGS(max)) \ >> + continue; \ >> + } while (0) > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixed a macro coding style issue 2012-07-25 5:37 ` Dmitry Torokhov 2012-07-25 6:09 ` Baodong Chen @ 2012-07-25 6:15 ` Al Viro 2012-07-25 6:36 ` Dmitry Torokhov 1 sibling, 1 reply; 7+ messages in thread From: Al Viro @ 2012-07-25 6:15 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Baodong Chen, linux-input, linux-kernel On Tue, Jul 24, 2012 at 10:37:55PM -0700, Dmitry Torokhov wrote: > On Wed, Jul 25, 2012 at 01:20:56PM +0800, Baodong Chen wrote: > > Fixed a coding style issue in driver/input/input.c > > > > Signed-off-by: Baodong Chen <chenbdchenbd@gmail.com> > > --- > > drivers/input/input.c | 6 ++++-- > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/input/input.c b/drivers/input/input.c > > index 8921c61..c96e983 100644 > > --- a/drivers/input/input.c > > +++ b/drivers/input/input.c > > @@ -845,11 +845,13 @@ int input_set_keycode(struct input_dev *dev, > > EXPORT_SYMBOL(input_set_keycode); > > > > #define MATCH_BIT(bit, max) \ > > + do { \ > > for (i = 0; i < BITS_TO_LONGS(max); i++) \ > > if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \ > > break; \ > > - if (i != BITS_TO_LONGS(max)) \ > > - continue; > > + if (i != BITS_TO_LONGS(max)) \ > > + continue; \ > > + } while (0) > > 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixed a macro coding style issue 2012-07-25 6:15 ` Al Viro @ 2012-07-25 6:36 ` Dmitry Torokhov 2012-07-31 7:27 ` Dmitry Torokhov 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2012-07-25 6:36 UTC (permalink / raw) To: Al Viro; +Cc: Baodong Chen, linux-input, linux-kernel On Wed, Jul 25, 2012 at 07:15:12AM +0100, Al Viro wrote: > On Tue, Jul 24, 2012 at 10:37:55PM -0700, Dmitry Torokhov wrote: > > On Wed, Jul 25, 2012 at 01:20:56PM +0800, Baodong Chen wrote: > > > Fixed a coding style issue in driver/input/input.c > > > > > > Signed-off-by: Baodong Chen <chenbdchenbd@gmail.com> > > > --- > > > drivers/input/input.c | 6 ++++-- > > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/input/input.c b/drivers/input/input.c > > > index 8921c61..c96e983 100644 > > > --- a/drivers/input/input.c > > > +++ b/drivers/input/input.c > > > @@ -845,11 +845,13 @@ int input_set_keycode(struct input_dev *dev, > > > EXPORT_SYMBOL(input_set_keycode); > > > > > > #define MATCH_BIT(bit, max) \ > > > + do { \ > > > for (i = 0; i < BITS_TO_LONGS(max); i++) \ > > > if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \ > > > break; \ > > > - if (i != BITS_TO_LONGS(max)) \ > > > - continue; > > > + if (i != BITS_TO_LONGS(max)) \ > > > + continue; \ > > > + } while (0) > > > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] fixed a macro coding style issue 2012-07-25 6:36 ` Dmitry Torokhov @ 2012-07-31 7:27 ` Dmitry Torokhov 0 siblings, 0 replies; 7+ messages in thread From: Dmitry Torokhov @ 2012-07-31 7:27 UTC (permalink / raw) To: Al Viro; +Cc: Baodong Chen, linux-input, linux-kernel On Tue, Jul 24, 2012 at 11:36:24PM -0700, Dmitry Torokhov wrote: > On Wed, Jul 25, 2012 at 07:15:12AM +0100, Al Viro wrote: > > On Tue, Jul 24, 2012 at 10:37:55PM -0700, Dmitry Torokhov wrote: > > > On Wed, Jul 25, 2012 at 01:20:56PM +0800, Baodong Chen wrote: > > > > Fixed a coding style issue in driver/input/input.c > > > > > > > > Signed-off-by: Baodong Chen <chenbdchenbd@gmail.com> > > > > --- > > > > drivers/input/input.c | 6 ++++-- > > > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/input/input.c b/drivers/input/input.c > > > > index 8921c61..c96e983 100644 > > > > --- a/drivers/input/input.c > > > > +++ b/drivers/input/input.c > > > > @@ -845,11 +845,13 @@ int input_set_keycode(struct input_dev *dev, > > > > EXPORT_SYMBOL(input_set_keycode); > > > > > > > > #define MATCH_BIT(bit, max) \ > > > > + do { \ > > > > for (i = 0; i < BITS_TO_LONGS(max); i++) \ > > > > if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \ > > > > break; \ > > > > - if (i != BITS_TO_LONGS(max)) \ > > > > - continue; > > > > + if (i != BITS_TO_LONGS(max)) \ > > > > + continue; \ > > > > + } while (0) > > > > > > 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... 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 <chenbdchenbd@gmail.com> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/input.c | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/drivers/input/input.c b/drivers/input/input.c index 8921c61..768e46b 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -844,18 +844,10 @@ int input_set_keycode(struct input_dev *dev, } EXPORT_SYMBOL(input_set_keycode); -#define MATCH_BIT(bit, max) \ - for (i = 0; i < BITS_TO_LONGS(max); i++) \ - if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \ - break; \ - if (i != BITS_TO_LONGS(max)) \ - continue; - static const struct input_device_id *input_match_device(struct input_handler *handler, struct input_dev *dev) { const struct input_device_id *id; - int i; for (id = handler->id_table; id->flags || id->driver_info; id++) { @@ -875,15 +867,32 @@ static const struct input_device_id *input_match_device(struct input_handler *ha if (id->version != dev->id.version) continue; - MATCH_BIT(evbit, EV_MAX); - MATCH_BIT(keybit, KEY_MAX); - MATCH_BIT(relbit, REL_MAX); - MATCH_BIT(absbit, ABS_MAX); - MATCH_BIT(mscbit, MSC_MAX); - MATCH_BIT(ledbit, LED_MAX); - MATCH_BIT(sndbit, SND_MAX); - MATCH_BIT(ffbit, FF_MAX); - MATCH_BIT(swbit, SW_MAX); + if (!bitmap_subset(id->evbit, dev->evbit, EV_MAX)) + continue; + + if (!bitmap_subset(id->keybit, dev->keybit, KEY_MAX)) + continue; + + if (!bitmap_subset(id->relbit, dev->relbit, REL_MAX)) + continue; + + if (!bitmap_subset(id->absbit, dev->absbit, ABS_MAX)) + continue; + + if (!bitmap_subset(id->mscbit, dev->mscbit, MSC_MAX)) + continue; + + if (!bitmap_subset(id->ledbit, dev->ledbit, LED_MAX)) + continue; + + if (!bitmap_subset(id->sndbit, dev->sndbit, SND_MAX)) + continue; + + if (!bitmap_subset(id->ffbit, dev->ffbit, FF_MAX)) + continue; + + if (!bitmap_subset(id->swbit, dev->swbit, SW_MAX)) + continue; if (!handler->match || handler->match(handler, dev)) return id; ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-07-31 7:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <no>
2012-07-25 5:20 ` [PATCH] fixed a macro coding style issue Baodong Chen
2012-07-25 5:27 ` Venu Byravarasu
2012-07-25 5:37 ` Dmitry Torokhov
2012-07-25 6:09 ` Baodong Chen
2012-07-25 6:15 ` Al Viro
2012-07-25 6:36 ` Dmitry Torokhov
2012-07-31 7:27 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).