From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH 0/6] Remove some usage of shadow variable Date: Wed, 28 Oct 2015 10:12:20 +0100 Message-ID: <56309174.8010608@suse.com> References: <1445960359-17217-1-git-send-email-julien.grall@citrix.com> <562FAE5002000078000AF3C2@prv-mh.provo.novell.com> <56309AFF02000078000AF718@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZrMus-0001O0-53 for xen-devel@lists.xenproject.org; Wed, 28 Oct 2015 09:21:10 +0000 In-Reply-To: <56309AFF02000078000AF718@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , George Dunlap Cc: KeirFraser , IanCampbell , Dario Faggioli , Tim Deegan , Julien Grall , Stefano Stabellini , xen-devel List-Id: xen-devel@lists.xenproject.org On 10/28/2015 09:53 AM, Jan Beulich wrote: >>>> On 27.10.15 at 18:41, wrote: >> On Tue, Oct 27, 2015 at 4:03 PM, Jan Beulich wrote: >>>>>> On 27.10.15 at 16:39, wrote: >>>> I'd like to have some input to know whether turning on -Wshadow would = be >>>> sensible in the future. >>> >>> I think there are cases where using a shadowed variable might make >>> sense, and hence I wouldn't want to see the warning turned on by >>> default. >> >> Hmm, I'm having trouble coming up with good uses off the top of my >> head. And are there any uses for which the value outweighs the value >> of having the warning? > > First of all - macros using the ({}) gcc extension. This case is easy to avoid: name local variables prefixed with the macro name. While this isn't true today, I think this would be a good policy for future changes. > And second variables > whose name is kind of natural (e.g. "d" for struct domain * instances) > but which are intentionally shadowing a larger scope one in order to > not clobber that one's value. Hmm, wouldn't something like tmp_d or d_tmp be a proper solution? There are other cases where more than one domain reference are needed and it was possible to find proper variable names. >> And in line with my response to Andrew -- could we enable -Wshadow >> until we find a use for shadowing whose value outweighs the risks of >> building without it? I have seen hard to find bugs before which were caused by shadowing variables. Shadowing variables even on purpose is something we should avoid IMO. > Risking - along the lines of what Andrew said - build breakage for > random people, just due to the gcc version they happen to use? > I'm usually getting pretty upset when running into problems specific > to certain gcc versions, where people fairly clearly didn't think about > making their code sufficiently general. I don't know how people will > feel if we intentionally break their build (well, not really intentionall= y, > but we'd intentionally take the risk of doing so). Is it really true that an older gcc might barf while a new one doesn't in case of shadowing? I don't think so. A test build with -Wshadow using the most recent gcc succeeding should do so with an older gcc as well. > And then, simply based on the patches that Julien sent so far: Are > we suspecting any bugs because of shadowing variables? None of > his patches fixed anything; they were just cleanup for cases where > the shadowing was pointless (and perhaps not even intended). Risking to repeat myself: I think shadowing on purpose is evil and leads to code which is harder to maintain. Just my 0.02=80 Juergen