On 04/25/2014 03:54 AM, Mike Stump wrote:
On Apr 24, 2014, at 4:16 PM, Dimitris Papavasiliou<dpapa...@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?
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
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
