This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 0/8] Add -Wshadow=local
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Tom Tromey <tom at tromey dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 3 Oct 2018 16:15:21 +0000
- Subject: Re: [PATCH 0/8] Add -Wshadow=local
- References: <20180923040814.27941-1-tom@tromey.com> <29c17639-6ee6-b089-be95-227f98ac0ccf@ericsson.com>
On 2018-10-03 11:01 AM, Simon Marchi wrote:
> On 2018-09-23 12:08 AM, Tom Tromey wrote:
>> This series enables -Wshadow=local for gdb.
>>
>> I think that shadowing is mildly confusing, and I normally try to
>> avoid it. However, for a long time, it wasn't feasible to enable any
>> shadowing warnings, because gcc would warn about shadowing that was
>> "uninteresting" -- the classic example being the use of a local
>> variable named "index".
>>
>> In this series I chose to use -Wshadow=local, to forbid shadowing
>> within a function. -Wshadow=compatible-local is another choice, but I
>> think that the stated semantics:
>>
>> In C++, type compatibility here means the type of the shadowing
>> variable can be converted to that of the shadowed variable.
>>
>> ... is maybe insufficient. For example a situation where the two
>> different types are both convertible to a third type could be enough
>> to cause a bug. That's a bit of a reach maybe, but it seemed to me
>> that it's not such a big deal, and certainly always safer, to just
>> disallow shadowing entirely.
>>
>> Adding this warning found a few latent bugs in gdb.
>>
>> Regression tested by the buildbot. Thanks, Sergio, for your work
>> running this -- it is a great service.
>>
>> Let me know what you think. I feel that a change like this should
>> involve some feedback.
>
> I think that's a good idea, I think variable shadowing is more likely
> done by mistake than anything else.
>
> Simon
>
I looked at the code, it all LGTM. As I said in my other message, I think
it would be easy to miss subtle mistakes when reviewing, given the repetitive
nature of the patch, but I don't really see a better/safer approach to do the
change. I think it will be a net positive.
Simon