From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25757 invoked by alias); 25 Nov 2011 00:48:10 -0000 Received: (qmail 25748 invoked by uid 22791); 25 Nov 2011 00:48:09 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-iy0-f169.google.com (HELO mail-iy0-f169.google.com) (209.85.210.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 25 Nov 2011 00:47:55 +0000 Received: by iaek3 with SMTP id k3so4977079iae.0 for ; Thu, 24 Nov 2011 16:47:54 -0800 (PST) MIME-Version: 1.0 Received: by 10.50.207.38 with SMTP id lt6mr35404669igc.43.1322182074848; Thu, 24 Nov 2011 16:47:54 -0800 (PST) Received: by 10.50.186.228 with HTTP; Thu, 24 Nov 2011 16:47:54 -0800 (PST) In-Reply-To: <20111124220057.GU13809@adacore.com> References: <201111231640.pANGefc4031803@d06av02.portsmouth.uk.ibm.com> <201111231820.40486.pedro@codesourcery.com> <201111232023.pANKNcLf022983@glazunov.sibelius.xs4all.nl> <20111124220057.GU13809@adacore.com> Date: Fri, 25 Nov 2011 00:48:00 -0000 Message-ID: Subject: Re: [PATCH 18/348] Fix -Wsahdow warnings From: Andrey Smirnov To: Joel Brobecker Cc: gdb-patches Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-11/txt/msg00691.txt.bz2 On Fri, Nov 25, 2011 at 5:00 AM, Joel Brobecker wro= te: > Andrey, > > I understand your fustration towards the tone of some of the messages. > Hopefully things will be better from now on. > > And I agree that not enabling -Wshadow by default will let the number > of such conflict increase again over time. But at the same time, now > that I am seeing the changes that are required to fix these, I am not > very happy either. =A0I mean, I understand that "index" might be part of > a system's include. > But "block_found" (or was it "found_block") seems > quite surprising. Thanks for bringing that up, that is, IMHO, an excellent example. The `block_found' in function conflicts with global variable declared in symtab.c and it is not that just their names match, types are very similar too one is `struct block **' another is `const struct *block'. But for the sake of argument let's assume that this will never cause any bugs. I say this shadowing still should be resolved because it makes source code, at least in my experience, harder to understand: on one hand in `ada_lookup_encoded_symbol' `block_found' is function parameter used to return result, on the other hand in `standard_lookup' it is a global variable from symtab.c, rather confusing. I say the whole situation is exactly why -Wshadow should be enabled. > Add the fact that includes and compiler vary from > system to system, and we're not sure that once clean on one machine, > it'll be clean everywhere else. All of this to fix warnings that, > as far as I could tell for the most part, did not indicate an actual > bug in the code. I've been humbled by the compiler and the machine too many times to think my bug spotting capabilities for all intents and purposes are any greater than zero, so my perspective at this is buggy until formally proven to be bug-free. And from my view your argument sounds really different: It means that we potentially can have a bug that is only reproducible on a certain mix of a compiler and the platform, now I don't mind fixing very well reproducible bugs, it this kind of once-in-a-blue-moon kind of bugs that I hate very passionately. > This is why I am left wondering (meaning I haven't decided yet) > whether the idea of enabling -Wshadow was such a good idea after > all. I know that looking at the warnings allowed you to spot some > areas where there definitely is a mistake, and so that's useful. > I'm not disputing that. But that tool is only available if the source tree in question can be compiled with -Wshadow, otherwise it is "checking for possible mistakes up until, in the order of source file compilation, the first legitimate shadowing". > But I'm not convinced by a good number of the patches I've seen, and > I still haven't decided whether to accept the situation and approve > them, or not. For that, I asked everyone else' opinion. Well, now that I made my case, I hope you reserve negative judgment until the whole patchset is presented. Andrey Smirnov.