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

[Juergen Gross <jgross@xxxxxxxx>]

On 10/28/2015 09:53 AM, 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.
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 intentionally,
> 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€


Juergen


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