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

[Juergen Gross <jgross@suse.com>]

On 10/28/2015 09:53 AM, Jan Beulich wrote:
>>>> On 27.10.15 at 18:41, <George.Dunlap@eu.citrix.com> wrote:
>> On Tue, Oct 27, 2015 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 27.10.15 at 16:39, <julien.grall@citrix.com> 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