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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Oct 28, 2015 at 8:53 AM, Jan Beulich <JBeulich@xxxxxxxx> 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.

Right -- so if we turn on -Wshadow, then it breaks the nice
"abstraction" provided by macros, since now suddenly calling any macro
might give you a random warning because the variable it used happened
to be the same as the variable name you wanted to use.  And you're
suggest elsewhere that simply adding an underscore or two at the front
is suboptimal, since that is reserved for language and library
constructs.  That is certainly an amount of hassle that's worth taking
into account.

>  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.

This one, however, I think is probably something we should try to
avoid doing.  If there's a global d which is set to some domain, and
you have a local variable you want to point to a different domain,
then there must be something different about the two domains -- in
which case giving it a different variable name (e.g., "rd" for "remote
domain") isn't really that much of a burden, and certainly makes the
code easier to understand and less likely to introduce the kind of
bugs that Julien described.

>> 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).

I don't really see the difference between this and what we do now with
regard to other warning options.  Every few months someone reports
some issue or other wrt compilers throwing out errors, and we either
change the code so it works with that version, or provide a
work-around.

> 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).

So your suggestion is, rather than wait until we find a positive
example where -Wshadow breaks something, we should wait until we find
a positive example where the lack of -Wshadow breaks something?

[snip from another email]
> I do
> appreciate the basic rule of writing simple code whenever possible,
> but I don't think this should go to the extent of forbidding anything
> more complicated (and hence more difficult to understand).

It's not so much about forbidding things that are complicated, but
considering the cost/benefits trade-off.  So far the benefits of
shadowing are:

* Macros that declare local variables are properly abstracted: you
don't need to worry about conflicts between variables at the call site
accidentally matching variables inside the declaration

* You can use a metavariable like "d" in a context where "d" is
already defined at a higher level, instead of giving it a different
name, like "rd".

* We avoid the risk that maybe somewhere there might be a gcc version
where variable shadow detection is broken.

And the costs are

* Risk of bugs where variables that used to be shadowed suddenly
become un-shadowed and vice versa

* Risk of bugs where people didn't realize that a particular variable
was being shadowed

On the whole, absent a specific example where gcc is completely broken
wrt -Wshadow, I'm inclined to agree with Julien, that the cost of
shadowing is higher than the benefits.

 -George

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