Re: [Xen-devel] [PATCH 0/6] Remove some usage of shadow variable

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Jan,

On 28/10/2015 08:53, Jan Beulich wrote:
> > > > On 27.10.15 at 18:41, <George.Dunlap@xxxxxxxxxxxxx> wrote:
> > On Tue, Oct 27, 2015 at 4:03 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > > > > > On 27.10.15 at 16:39, <julien.grall@xxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel