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

[Julien Grall <julien.grall@citrix.com>]

Hi Jan,

On 28/10/2015 08:53, 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. 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