From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH 0/6] Remove some usage of shadow variable Date: Wed, 28 Oct 2015 09:10:07 +0000 Message-ID: <563090EF.6040006@citrix.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="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZrMlX-0000sC-0w for xen-devel@lists.xenproject.org; Wed, 28 Oct 2015 09:11:31 +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 , Stefano Stabellini , xen-devel List-Id: xen-devel@lists.xenproject.org Hi Jan, On 28/10/2015 08:53, 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. 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. IHMO using variable is a very bad practice. It make patch review more difficult as you need to check that you effectively modify the correct version of the variable. I've got in mind patch #3 where we have something like that: int frame; [...] if ( foo ) { int frame; frame = smth; } frame = smth; While I agree that shadow variable could make sense in macros using ({}), all the variables in it are prefixed with _ and we can ensure that the variable name is never re-used in other variables. For instance min_t and max_t are using _x/_y. We could rename _min1/_min2 for min_t and then _max1/_max2 for max_t. >> 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? > > 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 intentionally, > but we'd intentionally take the risk of doing so). > > 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). FWIW, I haven't yet removed all the shadow variables because the other are more complex to understand. My main concern with shadow variables is we may introduce at some point a bug because the patch doesn't show which version of the variable you are modifying. Worth, you may not spot there is a shadow version even if you apply the patch to the tree and look at the code. -- Julien Grall