Message ID | 1276547208-26569-7-git-send-email-justinmattock@gmail.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Mon, 14 Jun 2010 20:28:21 +0000 Received: from bombadil.infradead.org [18.85.46.34] by localhost with IMAP (fetchmail-6.3.17) for <mchehab@localhost> (single-drop); Tue, 15 Jun 2010 07:59:47 +0300 (EEST) Received: from vger.kernel.org ([209.132.180.67]) by bombadil.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1OOGGb-0004yF-G9; Mon, 14 Jun 2010 20:28:21 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756168Ab0FNU1u (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Mon, 14 Jun 2010 16:27:50 -0400 Received: from mail-pv0-f174.google.com ([74.125.83.174]:52855 "EHLO mail-pv0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755730Ab0FNU1A (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 14 Jun 2010 16:27:00 -0400 Received: by mail-pv0-f174.google.com with SMTP id 16so3067678pvg.19 for <multiple recipients>; Mon, 14 Jun 2010 13:27:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:from:to:cc:subject:date :message-id:x-mailer:in-reply-to:references; bh=LtHcOOFr4DspdO5sAGLsW1SU5XU1E3fMqXGj6XU/UZs=; b=X9KTg0z4JxChHg3gRXRzXdd/Bmao81e55AUQNg7sKbkXIZO/ta1ScJaloSLAj4UClq FbfiNNaf5/5be8+T0nh0M0hy/hczrM+/H4ptM8GQJTbIX/13qHmh5diBMoQVKvFImGY7 eOOyv4CirbSLPkifeG8tDthUi1CyrKR6gz7To= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references; b=kXhO1lj5zobz8Bqbhyhyy0OZbuaCq4JnW4wjgmTS9EWGVrB4jz6RJpaSuELIRdIkd2 Nx8H4dTjsBEjVcSUpdxKY9rELMVINZa+WYngD2Qi7ytBNptvKMmfHeE5ZSsx+aopS68b kQizR+GWCiqbVd1HJLkGliiy7NFPS0jxdlsM4= Received: by 10.142.10.27 with SMTP id 27mr4319772wfj.265.1276547220007; Mon, 14 Jun 2010 13:27:00 -0700 (PDT) Received: from localhost.localdomain ([76.91.45.220]) by mx.google.com with ESMTPS id r20sm58982854wam.5.2010.06.14.13.26.58 (version=TLSv1/SSLv3 cipher=RC4-MD5); Mon, 14 Jun 2010 13:26:59 -0700 (PDT) From: "Justin P. Mattock" <justinmattock@gmail.com> To: linux-kernel@vger.kernel.org Cc: reiserfs-devel@vger.kernel.org, linux-bluetooth@vger.kernel.org, clemens@ladisch.de, debora@linux.vnet.ibm.com, dri-devel@lists.freedesktop.org, linux-i2c@vger.kernel.org, linux1394-devel@lists.sourceforge.net, linux-media@vger.kernel.org, "Justin P. Mattock" <justinmattock@gmail.com> Subject: [PATCH 6/8]i2c:i2c_core Fix warning: variable 'dummy' set but not used Date: Mon, 14 Jun 2010 13:26:46 -0700 Message-Id: <1276547208-26569-7-git-send-email-justinmattock@gmail.com> X-Mailer: git-send-email 1.7.1.rc1.21.gf3bd6 In-Reply-To: <1276547208-26569-1-git-send-email-justinmattock@gmail.com> References: <1276547208-26569-1-git-send-email-justinmattock@gmail.com> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Justin P. Mattock
June 14, 2010, 8:26 p.m. UTC
could be a right solution, could be wrong
here is the warning:
CC drivers/i2c/i2c-core.o
drivers/i2c/i2c-core.c: In function 'i2c_register_adapter':
drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
drivers/i2c/i2c-core.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
Comments
Hi Justin, On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: > could be a right solution, could be wrong > here is the warning: > CC drivers/i2c/i2c-core.o > drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': > drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used > > Signed-off-by: Justin P. Mattock <justinmattock@gmail.com> > > --- > drivers/i2c/i2c-core.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 1cca263..79c6c26 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) > mutex_lock(&core_lock); > dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, > __process_new_adapter); > + if(!dummy) > + dummy = 0; One word: scripts/checkpatch.pl In other news, the above is just plain wrong. First we force people to read the result of bus_for_each_drv() and then when they do and don't need the value, gcc complains, so we add one more layer of useless code, which developers and possibly tools will later wonder and complain about? I can easily imagine that a static code analyzer would spot the above code as being a potential bug. Let's stop this madness now please. Either __must_check goes away from bus_for_each_drv() and from every other function which raises this problem, or we must disable that new type of warning gcc 4.6.0 generates. Depends which warnings we value more, as we can't sanely have both. > mutex_unlock(&core_lock); > > return 0;
On 06/14/2010 01:53 PM, Jean Delvare wrote: > Hi Justin, > > On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: >> could be a right solution, could be wrong >> here is the warning: >> CC drivers/i2c/i2c-core.o >> drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': >> drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used >> >> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com> >> >> --- >> drivers/i2c/i2c-core.c | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >> index 1cca263..79c6c26 100644 >> --- a/drivers/i2c/i2c-core.c >> +++ b/drivers/i2c/i2c-core.c >> @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) >> mutex_lock(&core_lock); >> dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, >> __process_new_adapter); >> + if(!dummy) >> + dummy = 0; > > One word: scripts/checkpatch.pl it was this, and/or just take the code out (since I'm a newbie) > > In other news, the above is just plain wrong. First we force people to > read the result of bus_for_each_drv() and then when they do and don't > need the value, gcc complains, so we add one more layer of useless > code, which developers and possibly tools will later wonder and > complain about? I can easily imagine that a static code analyzer would > spot the above code as being a potential bug. > > Let's stop this madness now please. > your telling me!! I haven't even compiled all the way through the kernel yet.(lots of warnings) > Either __must_check goes away from bus_for_each_drv() and from every > other function which raises this problem, or we must disable that new > type of warning gcc 4.6.0 generates. Depends which warnings we value > more, as we can't sanely have both. > >> mutex_unlock(&core_lock); >> >> return 0; > > up to you guys.. best thing now is deciphering what and what not is an actual issue. Justin P. Mattock -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/14/2010 01:53 PM, Jean Delvare wrote: > Hi Justin, > > On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: >> could be a right solution, could be wrong >> here is the warning: >> CC drivers/i2c/i2c-core.o >> drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': >> drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used >> >> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com> >> >> --- >> drivers/i2c/i2c-core.c | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >> index 1cca263..79c6c26 100644 >> --- a/drivers/i2c/i2c-core.c >> +++ b/drivers/i2c/i2c-core.c >> @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) >> mutex_lock(&core_lock); >> dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, >> __process_new_adapter); >> + if(!dummy) >> + dummy = 0; > > One word: scripts/checkpatch.pl > > In other news, the above is just plain wrong. First we force people to > read the result of bus_for_each_drv() and then when they do and don't > need the value, gcc complains, so we add one more layer of useless > code, which developers and possibly tools will later wonder and > complain about? I can easily imagine that a static code analyzer would > spot the above code as being a potential bug. > > Let's stop this madness now please. > > Either __must_check goes away from bus_for_each_drv() and from every > other function which raises this problem, or we must disable that new > type of warning gcc 4.6.0 generates. Depends which warnings we value > more, as we can't sanely have both. > That is the crux of the whole thing. Putting in crap to get rid of the __must_check warning someone obviously wanted to provoke is just plain wrong. I don't know what the answer is, but in addition to your suggestion of removing the __must_check, you might try: BUG_ON(dummy != WHAT_IT_SHOULD_BE); or if (dummy != WHAT_IT_SHOULD_BE) panic("nice message here); or static inline void i_really_know_what_i_am_doing(int arg) { /* * Trick the compiler because we don't want to * handle error conditions. */ return; } . . . i_really_know_what_i_am_doing(dummy); David Daney -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David, On Mon, 14 Jun 2010 14:28:57 -0700, David Daney wrote: > On 06/14/2010 01:53 PM, Jean Delvare wrote: > > Hi Justin, > > > > On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: > >> could be a right solution, could be wrong > >> here is the warning: > >> CC drivers/i2c/i2c-core.o > >> drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': > >> drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used > >> > >> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com> > >> > >> --- > >> drivers/i2c/i2c-core.c | 2 ++ > >> 1 files changed, 2 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > >> index 1cca263..79c6c26 100644 > >> --- a/drivers/i2c/i2c-core.c > >> +++ b/drivers/i2c/i2c-core.c > >> @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) > >> mutex_lock(&core_lock); > >> dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, > >> __process_new_adapter); > >> + if(!dummy) > >> + dummy = 0; > > > > One word: scripts/checkpatch.pl > > > > In other news, the above is just plain wrong. First we force people to > > read the result of bus_for_each_drv() and then when they do and don't > > need the value, gcc complains, so we add one more layer of useless > > code, which developers and possibly tools will later wonder and > > complain about? I can easily imagine that a static code analyzer would > > spot the above code as being a potential bug. > > > > Let's stop this madness now please. > > > > Either __must_check goes away from bus_for_each_drv() and from every > > other function which raises this problem, or we must disable that new > > type of warning gcc 4.6.0 generates. Depends which warnings we value > > more, as we can't sanely have both. > > > > That is the crux of the whole thing. Putting in crap to get rid of the > __must_check warning someone obviously wanted to provoke is just plain > wrong. __process_new_adapter() calls i2c_do_add_adapter() which always returns 0. Why should I check the return value of bus_for_each_drv() when I know it will always be 0 by construction? Also note that the same function is also called through bus_for_each_dev() somewhere else in i2c-core, and there is no warning there because bus_for_each_dev() is not marked __must_check. How consistent is this? If bus_for_each_dev() is OK without __must_check, then I can't see why bus_for_each_drv() wouldn't be. > I don't know what the answer is, but in addition to your suggestion of > removing the __must_check, you might try: > > BUG_ON(dummy != WHAT_IT_SHOULD_BE); > > or > > if (dummy != WHAT_IT_SHOULD_BE) > panic("nice message here); Which will never trigger. > or > > static inline void i_really_know_what_i_am_doing(int arg) > { > /* > * Trick the compiler because we don't want to > * handle error conditions. > */ > return; > } > > .. > .. > .. > > i_really_know_what_i_am_doing(dummy); Which is adding a lot of lines, and might eventually fail when the compiler becomes smarter (if it isn't already). Thanks but no thanks. If I really have to chose one of these evils, I'll go for BUG_ON(), at least the intent is clear and the bloat is minimum.
Hi Justin, On Mon, 14 Jun 2010 14:06:12 -0700, Justin P. Mattock wrote: > On 06/14/2010 01:53 PM, Jean Delvare wrote: > > Hi Justin, > > > > On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: > >> could be a right solution, could be wrong > >> here is the warning: > >> CC drivers/i2c/i2c-core.o > >> drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': > >> drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used > >> > >> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com> > >> > >> --- > >> drivers/i2c/i2c-core.c | 2 ++ > >> 1 files changed, 2 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > >> index 1cca263..79c6c26 100644 > >> --- a/drivers/i2c/i2c-core.c > >> +++ b/drivers/i2c/i2c-core.c > >> @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) > >> mutex_lock(&core_lock); > >> dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, > >> __process_new_adapter); > >> + if(!dummy) > >> + dummy = 0; > > > > One word: scripts/checkpatch.pl > > it was this, and/or just take the code out > (since I'm a newbie) I was not (yet) arguing on the code itself, but on its format. Any patch you send should pass the formatting tests performed by scripts/checkpatch.pl. Thanks.
On 06/15/2010 04:40 AM, Jean Delvare wrote: > Hi David, > > On Mon, 14 Jun 2010 14:28:57 -0700, David Daney wrote: >> On 06/14/2010 01:53 PM, Jean Delvare wrote: >>> Hi Justin, >>> >>> On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: >>>> could be a right solution, could be wrong >>>> here is the warning: >>>> CC drivers/i2c/i2c-core.o >>>> drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': >>>> drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used >>>> >>>> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com> >>>> >>>> --- >>>> drivers/i2c/i2c-core.c | 2 ++ >>>> 1 files changed, 2 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >>>> index 1cca263..79c6c26 100644 >>>> --- a/drivers/i2c/i2c-core.c >>>> +++ b/drivers/i2c/i2c-core.c >>>> @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) >>>> mutex_lock(&core_lock); >>>> dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, >>>> __process_new_adapter); >>>> + if(!dummy) >>>> + dummy = 0; >>> >>> One word: scripts/checkpatch.pl >>> >>> In other news, the above is just plain wrong. First we force people to >>> read the result of bus_for_each_drv() and then when they do and don't >>> need the value, gcc complains, so we add one more layer of useless >>> code, which developers and possibly tools will later wonder and >>> complain about? I can easily imagine that a static code analyzer would >>> spot the above code as being a potential bug. >>> >>> Let's stop this madness now please. >>> >>> Either __must_check goes away from bus_for_each_drv() and from every >>> other function which raises this problem, or we must disable that new >>> type of warning gcc 4.6.0 generates. Depends which warnings we value >>> more, as we can't sanely have both. >>> >> >> That is the crux of the whole thing. Putting in crap to get rid of the >> __must_check warning someone obviously wanted to provoke is just plain >> wrong. > > __process_new_adapter() calls i2c_do_add_adapter() which always returns > 0. Why should I check the return value of bus_for_each_drv() when I > know it will always be 0 by construction? > > Also note that the same function is also called through > bus_for_each_dev() somewhere else in i2c-core, and there is no warning > there because bus_for_each_dev() is not marked __must_check. How > consistent is this? If bus_for_each_dev() is OK without __must_check, > then I can't see why bus_for_each_drv() wouldn't be. > Well, I would advocate removing the __must_check then. David Daney -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/15/2010 04:43 AM, Jean Delvare wrote: > Hi Justin, > > On Mon, 14 Jun 2010 14:06:12 -0700, Justin P. Mattock wrote: >> On 06/14/2010 01:53 PM, Jean Delvare wrote: >>> Hi Justin, >>> >>> On Mon, 14 Jun 2010 13:26:46 -0700, Justin P. Mattock wrote: >>>> could be a right solution, could be wrong >>>> here is the warning: >>>> CC drivers/i2c/i2c-core.o >>>> drivers/i2c/i2c-core.c: In function 'i2c_register_adapter': >>>> drivers/i2c/i2c-core.c:757:15: warning: variable 'dummy' set but not used >>>> >>>> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com> >>>> >>>> --- >>>> drivers/i2c/i2c-core.c | 2 ++ >>>> 1 files changed, 2 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >>>> index 1cca263..79c6c26 100644 >>>> --- a/drivers/i2c/i2c-core.c >>>> +++ b/drivers/i2c/i2c-core.c >>>> @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) >>>> mutex_lock(&core_lock); >>>> dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, >>>> __process_new_adapter); >>>> + if(!dummy) >>>> + dummy = 0; >>> >>> One word: scripts/checkpatch.pl >> >> it was this, and/or just take the code out >> (since I'm a newbie) > > I was not (yet) arguing on the code itself, but on its format. Any > patch you send should pass the formatting tests performed by > scripts/checkpatch.pl. Thanks. > o.k. I'll make sure I run everything through checkpatch.pl before sending anything. Justin P. Mattock -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 15 Jun 2010 09:20:39 -0700, David Daney wrote: > On 06/15/2010 04:40 AM, Jean Delvare wrote: > > __process_new_adapter() calls i2c_do_add_adapter() which always returns > > 0. Why should I check the return value of bus_for_each_drv() when I > > know it will always be 0 by construction? > > > > Also note that the same function is also called through > > bus_for_each_dev() somewhere else in i2c-core, and there is no warning > > there because bus_for_each_dev() is not marked __must_check. How > > consistent is this? If bus_for_each_dev() is OK without __must_check, > > then I can't see why bus_for_each_drv() wouldn't be. > > Well, I would advocate removing the __must_check then. I have just sent a patch to LKML doing exactly this.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 1cca263..79c6c26 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -794,6 +794,8 @@ static int i2c_register_adapter(struct i2c_adapter *adap) mutex_lock(&core_lock); dummy = bus_for_each_drv(&i2c_bus_type, NULL, adap, __process_new_adapter); + if(!dummy) + dummy = 0; mutex_unlock(&core_lock); return 0;