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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.10.15 at 11:50, <George.Dunlap@xxxxxxxxxxxxx> wrote:
> 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:
>>> 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.

The main difference is that the reports you refer to result from
changes to the compiler, whereas you suggest enabling a previously
disabled warning, i.e. us taking active action that incurs this risk.

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

I wouldn't go _that_ far; I'm just wondering whether enabling the
warning would buy us anything (and enough to outweigh the hassle
it at least may cause).

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

No, that paints too nice a picture: The risk of problematic collisions
exists no matter how you write a macro. That's why people like to
prefix (or suffix, which at least doesn't clash with the language spec)
underscores to local variable names.

Jan


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