diff mbox

[PING^8] Add a couple of dialect and warning options regarding Objective-C instance variable scope

Message ID 535A47FB.7060800@gmail.com
State New
Headers show

Commit Message

Dimitris Papavasiliou April 25, 2014, 11:33 a.m. UTC 
On 04/25/2014 03:54 AM, Mike Stump wrote:
> On Apr 24, 2014, at 4:16 PM, Dimitris Papavasiliou<dpapavas@gmail.com>  wrote:
>> On 04/24/2014 11:17 PM, Jakub Jelinek wrote:
>>> How has this been tested?
>>>
>>> I'm seeing:
>>>
>>> +FAIL: obj-c++.dg/local-decl-1.mm -fgnu-runtime  (test for warnings, line 39)
>>> +FAIL: obj-c++.dg/local-decl-1.mm -fgnu-runtime  (test for warnings, line 41)
>>> +FAIL: obj-c++.dg/private-2.mm -fgnu-runtime  (test for warnings, line 32)
>>> +FAIL: obj-c++.dg/private-2.mm -fgnu-runtime  (test for warnings, line 33)
>>> +FAIL: obj-c++.dg/private-2.mm -fgnu-runtime  (test for warnings, line 34)
>>> +FAIL: obj-c++.dg/private-2.mm -fgnu-runtime  (test for warnings, line 53)
>>> +FAIL: obj-c++.dg/private-2.mm -fgnu-runtime  (test for warnings, line 54)
>>> +FAIL: objc.dg/local-decl-1.m -fgnu-runtime  (test for warnings, line 23)
>>> +FAIL: objc.dg/local-decl-2.m -fgnu-runtime  (test for warnings, line 37)
>>> +FAIL: objc.dg/local-decl-2.m -fgnu-runtime  (test for warnings, line 39)
>>> +FAIL: objc.dg/private-2.m -fgnu-runtime  (test for warnings, line 30)
>>> +FAIL: objc.dg/private-2.m -fgnu-runtime  (test for warnings, line 31)
>>> +FAIL: objc.dg/private-2.m -fgnu-runtime  (test for warnings, line 32)
>>> +FAIL: objc.dg/private-2.m -fgnu-runtime  (test for warnings, line 51)
>>> +FAIL: objc.dg/private-2.m -fgnu-runtime  (test for warnings, line 52)
>>>
>>> regressions compared to a bootstrap/regtest from a few hours ago on
>>> i686-linux (x86_64-linux still regtesting, but the objc.dg/ failures
>>> can be seen in the logs already).
>>>
>>> 	Jakub
>>>
>>
>> These failures are due to the fact that the patch made -Wshadow control the warnings related to instance variable hiding.  These were before "enabled by default" although they were clearly cases of variable shadowing.
>
> So, in general, to contribute patches, you will want to run the test suite to ensure quality.  We don’t want to remove warnings without good reason…  so I’m defaulting them back to on.  In your text, you didn’t seem to cover this, other than to note what we do and to say some might want to turn them off.
>
> Now, if clang turned them off by default, we can go that way, but I’d like to maintain users expectations of those on.  I’m anticipating they have them on by default still.

I tried to implement all changes in such a way that the default behavior 
of GCC wouldn't be changed but this side-effect slipped my attention.  I 
also tried to run the test suite but, as I recall, I was getting errors 
even on the freshly checked out sources, so I got lazy and gave up on 
that until the proposed changes were actually approved for merging. 
Sorry about that.

> Index: gcc/c-family/c.opt
> ===================================================================
> --- gcc/c-family/c.opt	(revision 209755)
> +++ gcc/c-family/c.opt	(working copy)
> @@ -685,7 +685,7 @@ ObjC ObjC++ Var(warn_selector) Warning
>   Warn if a selector has multiple methods
>
>   Wshadow-ivar
> -ObjC ObjC++ Var(warn_shadow_ivar) Init(-1) Warning
> +ObjC ObjC++ Var(warn_shadow_ivar) Init(1) Warning
>   Warn if a local declaration hides an instance variable
>
>   Wsequence-point
>
> Committed revision 209774.
>
> Thanks Jakub for pointing it out.

Although this will work to get the expected behavior back it has the 
slightly annoying side-effect that specifying -Wno-shadow explicitly 
will *not* turn this particular form of variable shadowing warning off. 
  Might I suggest the following change?


The GCC Internals documentation is a little vague as to what the value 
of warn_shadow_ivar will be if Init and EnabledBy are both specified, 
and -Wshadow is *not* explicitly specified, but tests indicate that we 
get the desired behavior.  That is, the warning is enabled by default 
but can be disabled both by -Wno-shadow and by -Wno-shadow-ivar.

If you adopt this change, let me know so that I can add a testcase for 
-Wno-shadow as well.

Dimitris
diff mbox

Patch

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 209787)
+++ gcc/c-family/c.opt	(working copy)
@@ -685,7 +685,7 @@ 
  Warn if a selector has multiple methods

  Wshadow-ivar
-ObjC ObjC++ Var(warn_shadow_ivar) Init(1) Warning
+ObjC ObjC++ Var(warn_shadow_ivar) EnabledBy(Wshadow) Init(1) Warning
  Warn if a local declaration hides an instance variable

  Wsequence-point