Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1339761848-7472-1-git-send-email-hdante@profusion.mobi> Date: Fri, 22 Jun 2012 18:10:16 -0300 Message-ID: Subject: Re: [PATCH RFC] gdbus: Rename variables named "signal" (so that it can be compiled with -Wshadow) From: Henrique Dante To: Joao Paulo Rechi Vita Cc: Lucas De Marchi , Anderson Lizardo , linux-bluetooth@vger.kernel.org, Johan Hedberg Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Any conclusions about this ? On Wed, Jun 20, 2012 at 10:59 AM, Joao Paulo Rechi Vita wrote: > On Wed, Jun 20, 2012 at 10:20 AM, Joao Paulo Rechi Vita > wrote: >> On Tue, Jun 19, 2012 at 6:41 PM, Lucas De Marchi >> wrote: >>> On Tue, Jun 19, 2012 at 3:29 PM, Joao Paulo Rechi Vita >>> wrote: >>>> On Fri, Jun 15, 2012 at 9:45 AM, Henrique Dante wrote: >>>>> On Fri, Jun 15, 2012 at 9:43 AM, Henrique Dante wrote: >>>>>> On Fri, Jun 15, 2012 at 9:29 AM, Anderson Lizardo >>>>>> wrote: >>>>>>> Hi Henrique, >>>>>>> >>>>>>> On Fri, Jun 15, 2012 at 8:04 AM, Henrique Dante de Almeida >>>>>>> wrote: >>>>>>>> --- >>>>>>>> ?gdbus/object.c | ? 39 +++++++++++++++++++++------------------ >>>>>>>> ?gdbus/watch.c ?| ? ?4 ++-- >>>>>>>> ?2 files changed, 23 insertions(+), 20 deletions(-) >>>>>>> >>>>>>> Would it be interesting to add this option to acinclude.m4? Or does it >>>>>>> generate too much noise? >>>>>> >>>>>> ?It generates few warnings. Depending on the acceptance of this patch, >>>>>> I could fix bluez as a whole and add -Wshadow to acinclude.m4. >>>>> >>>>> ?Actually, I had a partial build here. Ignore the previous answer, it >>>>> generates a lot of warnings. >>>>> >>>> >>>> If we're not going to enable -Wshadow by default, does it make sense >>>> to apply this patch? Who is going to check if no new shadow warnings >>>> are being inserted in new commits? >>> >>> I'm all for doing the following: >>> >>> 1) Fix all the places with shadow variables >>> 2) Add -Wshadow to the warning flags >>> >>> There are lots of them. >>> >> >> Yes, that makes sense. >> > > Or not completely, as researching a little bit on what kind of > warnings -Wshadow generates shows that not all of them are useful [1] > (I'm quoting the relevant part of this link below). I think before > getting our hands dirty to change this, we should look if we want to > live with all the restrictions imposed by it. Opinions? > > [1] http://kerneltrap.org/node/7434 > > """ > From: Linus Torvalds [email blocked] > Subject: Re: [PATCH] Don't compare unsigned variable for <0 in sys_prctl() > Date: ? Tue, 28 Nov 2006 16:13:05 -0800 (PST) > > > > On Wed, 29 Nov 2006, Jesper Juhl wrote: >> >> I would venture that "-Wshadow" is another one of those. > > I'd agree, except for the fact that gcc does a horribly _bad_ job of > -Wshadow, making it (again) totally unusable. > > For example, it's often entirely interesting to hear about local variables > that shadow each other. No question about it. > > HOWEVER. It's _not_ really interesting to hear about a local variable that > happens to have a common name that is also shared by a extern function. > > There just isn't any room for confusion, and it's actually not even that > unusual - I tried using -Wshadow on real programs, and it was just > horribly irritating. > > In the kernel, we had obvious things like local use of "jiffies" that just > make _total_ sense in a small inline function, and the fact that there > happens to be an extern declaration for "jiffies" just isn't very > interesting. > > Similarly, with nested macro expansion, even the "local variable shadows > another local variable" case - that looks like it should have an obvious > warning on the face of it - really isn't always necessarily that > interesting after all. Maybe it is a bug, maybe it isn't, but it's no > longer _obviously_ bogus any more. > > So I'm not convinced about the usefulness of "-Wshadow". ESPECIALLY the > way that gcc implements it, it's almost totally useless in real life. > > For example, I tried it on "git" one time, and this is a perfect example > of why "-Wshadow" is totally broken: > > ? ? ? ?diff-delta.c: In function 'create_delta_index': > ? ? ? ?diff-delta.c:142: warning: declaration of 'index' shadows a global declaration > > (and there's a _lot_ of those). If I'm not allowed to use "index" as a > local variable and include at the same time, something is > simply SERIOUSLY WRONG with the warning. > > So the fact is, the C language has scoping rules for a reason. Can you > screw yourself by usign them badly? Sure. But that does NOT mean that the > same name in different scopes is a bad thing that should be warned about. > > If I wanted a language that didn't allow me to do anything wrong, I'd be > using Pascal. As it is, it turns out that things that "look" wrong on a > local level are often not wrong after all. > > ? ? ? ? ? ? ? ? ? ? ? ?Linus > """ > > -- > Jo?o Paulo Rechi Vita > Openbossa Labs - INdT