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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.10.15 at 10:10, <julien.grall@xxxxxxxxxx> wrote:
> On 28/10/2015 08:53, Jan Beulich wrote:
>> 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.

Note that identifiers starting with an underscore and a lower case
letter have a different purpose as per the language spec. Yes,
macro variable names do generally get chosen to avoid clashes,
but as long as people also consider it right to name other variables,
function parameters or macro parameters in a similar way, we won't
be able to fully exclude the risk of shadowed variables. And namely
for macros with a very special purpose (and perhaps narrow scope)
I don't think considering possible shadowing issues really is
warranted.

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

Yes, there is a risk of bugs _solely_ because of shadowed variables.
But you realize that's no different from any other not exactly trivial
use of advanced language features (or quirks, if you take the
position of things like shadowed variables being a mis-feature)? 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).

Jan


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