Re: [PATCH] Add a couple of dialect and warning options regarding Objective-C instance variable scope

[Dimitris Papavasiliou <dpapavas@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

On 04/25/2014 07:50 PM, Mike Stump wrote:
> On Apr 25, 2014, at 9:34 AM, Dimitris Papavasiliou<dpapavas@gmail.com>  wrote:
>
>> --Wreturn-type  -Wsequence-point  -Wshadow @gol
>> +-Wreturn-type  -Wsequence-point  -Wshadow  -Wshadow-ivar @gol
>
> This has to be -Wno-shadow-ivar, we document the non-defaultÂ…
>
>> +@item -Wshadow-ivar @r{(Objective-C only)}
>
> Likewise.
>
>> +  /* Check wheter the local variable hides the instance variable. */
>
> spelling, whether...

Fixed these.

>> +  a = private;    /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
>> +  a = protected;  /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
>> +  a = public;     /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
>
> No, we donÂ’t expect failures.  We makes the compiler do what we wants or it gets the hose again.  Then, we expect it to be perfect.  If you wonÂ’t want warning, and non are produces, then just remove the /* Â… */, or write /* no warning */.

I've fixed these as per your request.  For the record though, this form 
of test seems to be fairly common in the test suites as this output 
indicates:

dimitris@debian:~/sandbox/gcc-build$ find ../gcc-source/gcc/testsuite/ 
-name "*.c" -o -name "*.C" -o -name "*.m" | xargs grep "xfail \*-\*-\*" 
| wc -l
354

Many of these seem to be in error or warning messages which are expected 
not to show up.  In any case if the messages do show up they'll trigger 
the excessive messages test so I suppose that's enough.

> Also, synth up the ChnageLogsÂ… :-), they are trivial enough.

Done.

> And, just pop them all into one patch (cd ..; svn diff), 3 is 3x the work for me.

Attached.

> Once we resolve the 3 warning tests above, this will be ok.

Actually, there were a few more { xfail *-*-* } in the other test cases. 
  I've removed these as well.

Dimitris


[-- Attachment #2: tests_and_docs.patch --]
[-- Type: text/x-patch, Size: 14192 bytes --]

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 209852)
+++ 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
Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 209852)
+++ gcc/doc/invoke.texi	(working copy)
@@ -216,6 +216,8 @@
 -fobjc-gc @gol
 -fobjc-nilcheck @gol
 -fobjc-std=objc1 @gol
+-fno-local-ivars @gol
+-fivar-visibility=@var{public|protected|private|package} @gol
 -freplace-objc-classes @gol
 -fzero-link @gol
 -gen-decls @gol
@@ -261,7 +263,7 @@
 -Wparentheses  -Wpedantic-ms-format -Wno-pedantic-ms-format @gol
 -Wpointer-arith  -Wno-pointer-to-int-cast @gol
 -Wredundant-decls  -Wno-return-local-addr @gol
--Wreturn-type  -Wsequence-point  -Wshadow @gol
+-Wreturn-type  -Wsequence-point  -Wshadow  -Wno-shadow-ivar @gol
 -Wsign-compare  -Wsign-conversion -Wfloat-conversion @gol
 -Wsizeof-pointer-memaccess @gol
 -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol
@@ -2976,6 +2978,22 @@
 The GNU runtime currently always retains calls to @code{objc_get_class("@dots{}")}
 regardless of command-line options.
 
+@item -fno-local-ivars
+@opindex fno-local-ivars
+@opindex flocal-ivars
+By default instance variables in Objective-C can be accessed as if
+they were local variables from within the methods of the class they're
+declared in.  This can lead to shadowing between instance variables
+and other variables declared either locally inside a class method or
+globally with the same name.  Specifying the @option{-fno-local-ivars}
+flag disables this behavior thus avoiding variable shadowing issues.
+
+@item -fivar-visibility=@var{public|protected|private|package}
+@opindex fivar-visibility
+Set the default instance variable visibility to the specified option
+so that instance variables declared outside the scope of any access
+modifier directives default to the specified visibility.
+
 @item -gen-decls
 @opindex gen-decls
 Dump interface declarations for all classes seen in the source file to a
@@ -4350,11 +4368,18 @@
 @item -Wshadow
 @opindex Wshadow
 @opindex Wno-shadow
-Warn whenever a local variable or type declaration shadows another variable,
-parameter, type, or class member (in C++), or whenever a built-in function
-is shadowed. Note that in C++, the compiler warns if a local variable
-shadows an explicit typedef, but not if it shadows a struct/class/enum.
+Warn whenever a local variable or type declaration shadows another
+variable, parameter, type, class member (in C++), or instance variable
+(in Objective-C) or whenever a built-in function is shadowed. Note
+that in C++, the compiler warns if a local variable shadows an
+explicit typedef, but not if it shadows a struct/class/enum.
 
+@item -Wno-shadow-ivar @r{(Objective-C only)}
+@opindex Wno-shadow-ivar
+@opindex Wshadow-ivar
+Do not warn whenever a local variable shadows an instance variable in an
+Objective-C method.
+
 @item -Wlarger-than=@var{len}
 @opindex Wlarger-than=@var{len}
 @opindex Wlarger-than-@var{len}
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 209852)
+++ gcc/ChangeLog	(working copy)
@@ -1,3 +1,11 @@
+2014-04-28  Dimitris Papavasiliou  <dpapavas@gmail.com>
+
+	* doc/invoke.texi: Document new switches -Wno-shadow-ivar,
+	-fno-local-ivars and -fivar-visibility.
+	* c-family/c.opt: Make -Wshadow also implicitly enable
+	-Wshadow-ivar.
+
+
 2014-04-28  Eric Botcazou  <ebotcazou@adacore.com>
 
 	* configure.ac: Tweak GAS check for LEON instructions on SPARC.
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 209852)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -1,3 +1,16 @@
+2014-04-28  Dimitris Papavasiliou  <dpapavas@gmail.com>
+
+	* objc.dg/shadow-1.m: New test.
+	* objc.dg/shadow-2.m: New test.
+	* objc.dg/ivar-scope-1.m: New test.
+	* objc.dg/ivar-scope-2.m: New test.
+	* objc.dg/ivar-scope-3.m: New test.
+	* objc.dg/ivar-scope-4.m: New test.
+	* objc.dg/ivar-visibility-1.m: New test.
+	* objc.dg/ivar-visibility-2.m: New test.
+	* objc.dg/ivar-visibility-3.m: New test.
+	* objc.dg/ivar-visibility-4.m: New test.
+
 2014-03-27  Thomas Koenig  <tkoenig@gcc.gnu.org>
 
 	PR fortran/59604
Index: gcc/testsuite/objc.dg/ivar-visibility-2.m
===================================================================
--- gcc/testsuite/objc.dg/ivar-visibility-2.m	(revision 0)
+++ gcc/testsuite/objc.dg/ivar-visibility-2.m	(revision 0)
@@ -0,0 +1,34 @@
+/* Test instance variable visibility.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do compile } */
+/* { dg-additional-options "-fivar-visibility=protected" } */
+#include <objc/objc.h>
+
+@interface MySuperClass
+{
+    int someivar;
+}
+@end
+
+@implementation MySuperClass
+@end
+
+
+@interface MyClass : MySuperClass 
+@end
+
+@implementation MyClass
+@end
+
+@interface MyOtherClass
+- (void) test: (MyClass *) object;
+@end
+
+@implementation MyOtherClass
+- (void) test: (MyClass *) object
+{
+  int a;
+
+  a = object->someivar;   /* { dg-error "instance variable .someivar. is declared protected" } */
+}
+@end
Index: gcc/testsuite/objc.dg/ivar-scope-1.m
===================================================================
--- gcc/testsuite/objc.dg/ivar-scope-1.m	(revision 0)
+++ gcc/testsuite/objc.dg/ivar-scope-1.m	(revision 0)
@@ -0,0 +1,24 @@
+/* Test instance variable scope.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do compile } */
+#include <objc/objc.h>
+
+@interface MyClass
+{
+  int someivar;
+}
+- (void) test;
+@end
+
+@implementation MyClass
+- (void) test
+{
+  int a;
+
+  /* Make sure instance variables do have local scope when
+     -fno-local-ivar isn't specified. */
+  
+  a = self->someivar;  /* No warning or error. */
+  a = someivar;        /* No error. */
+}
+@end
Index: gcc/testsuite/objc.dg/ivar-visibility-3.m
===================================================================
--- gcc/testsuite/objc.dg/ivar-visibility-3.m	(revision 0)
+++ gcc/testsuite/objc.dg/ivar-visibility-3.m	(revision 0)
@@ -0,0 +1,34 @@
+/* Test instance variable visibility.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do compile } */
+/* { dg-additional-options "-fivar-visibility=private" } */
+#include <objc/objc.h>
+
+@interface MySuperClass
+{
+    int someivar;
+}
+@end
+
+@implementation MySuperClass
+@end
+
+
+@interface MyClass : MySuperClass 
+@end
+
+@implementation MyClass
+@end
+
+@interface MyOtherClass
+- (void) test: (MyClass *) object;
+@end
+
+@implementation MyOtherClass
+- (void) test: (MyClass *) object
+{
+  int a;
+
+  a = object->someivar;   /* { dg-error "instance variable .someivar. is declared private" } */
+}
+@end
Index: gcc/testsuite/objc.dg/ivar-scope-2.m
===================================================================
--- gcc/testsuite/objc.dg/ivar-scope-2.m	(revision 0)
+++ gcc/testsuite/objc.dg/ivar-scope-2.m	(revision 0)
@@ -0,0 +1,34 @@
+/* Test instance variable scope.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do compile } */
+/* { dg-additional-options "-fno-local-ivars" } */
+#include <objc/objc.h>
+
+@interface MyClass
+{
+  int someivar;
+}
+- (void) testscope;
+- (void) testshadowing;
+@end
+
+@implementation MyClass
+- (void) testscope
+{
+  int a;
+
+  a = self->someivar;  /* No warning or error. */
+  a = someivar;        /* { dg-error ".someivar. undeclared" } */
+}
+
+- (void) testshadowing
+{
+  int someivar = 1;
+  int a;
+
+  /* Since instance variables don't have local scope no shadowing
+     should occur. */
+  
+  a = someivar; /* No warning. */
+}
+@end
Index: gcc/testsuite/objc.dg/ivar-visibility-4.m
===================================================================
--- gcc/testsuite/objc.dg/ivar-visibility-4.m	(revision 0)
+++ gcc/testsuite/objc.dg/ivar-visibility-4.m	(revision 0)
@@ -0,0 +1,36 @@
+/* Test instance variable visibility.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do compile } */
+/* { dg-additional-options "-fivar-visibility=public" } */
+#include <objc/objc.h>
+
+@interface MySuperClass
+{
+    int someivar;
+}
+@end
+
+@implementation MySuperClass
+@end
+
+
+@interface MyClass : MySuperClass 
+@end
+
+@implementation MyClass
+@end
+
+@interface MyOtherClass
+- (void) test: (MyClass *) object;
+@end
+
+@implementation MyOtherClass
+- (void) test: (MyClass *) object
+{
+  int a;
+
+  /* someivar is public so we shoudn't get any errors here. */
+  
+  a = object->someivar;
+}
+@end
Index: gcc/testsuite/objc.dg/ivar-scope-3.m
===================================================================
--- gcc/testsuite/objc.dg/ivar-scope-3.m	(revision 0)
+++ gcc/testsuite/objc.dg/ivar-scope-3.m	(revision 0)
@@ -0,0 +1,60 @@
+/* Test instance variable scope.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do run } */
+/* { dg-additional-options "-Wno-shadow-ivar" } */
+#include "../objc-obj-c++-shared/TestsuiteObject.m"
+#include <objc/objc.h>
+
+extern void abort(void);
+
+int someivar = 1;
+
+@interface MyClass: TestsuiteObject
+{
+  int someivar;
+}
+- (int) get;
+- (int) getHidden;
+@end
+
+@implementation MyClass
+- init
+{
+  someivar = 2;
+
+  return self;
+}
+
+- (int) get
+{
+  return someivar;
+}
+
+- (int) getHidden
+{
+  int someivar = 3;
+  
+  return someivar;
+}
+@end
+
+int main(void)
+{
+  MyClass *object;
+
+  object = [[MyClass alloc] init];
+
+  /* Check whether the instance variable hides the global variable. */
+  
+  if ([object get] != 2) {
+    abort();
+  }
+
+  /* Check whether the local variable hides the instance variable. */
+  
+  if ([object getHidden] != 3) {
+    abort();
+  }
+
+  return 0;
+}
Index: gcc/testsuite/objc.dg/ivar-scope-4.m
===================================================================
--- gcc/testsuite/objc.dg/ivar-scope-4.m	(revision 0)
+++ gcc/testsuite/objc.dg/ivar-scope-4.m	(revision 0)
@@ -0,0 +1,83 @@
+/* Test instance variable scope.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do run } */
+/* { dg-additional-options "-Wno-shadow-ivar -fno-local-ivars" } */
+#include "../objc-obj-c++-shared/runtime.h"
+#include <objc/objc.h>
+
+extern void abort(void);
+
+int someivar = 1;
+
+/* The testsuite object depends on local variable scope so we need to
+   implement our own minimal base object here. */
+
+@interface MyClass
+{
+  Class isa;
+  int someivar;
+}
+
++ (id) alloc;
+- (id) init;
+- (int) getGlobal;
+- (int) getInstance;
+- (int) getHidden;
+@end
+
+@implementation MyClass
++ (id) alloc
+{
+  return class_createInstance (self, 0);
+}
+
+- (id) init
+{
+  self->someivar = 2;
+
+  return self;
+}
+
+- (int) getGlobal
+{
+  return someivar;
+}
+
+- (int) getInstance
+{
+  return self->someivar;
+}
+
+- (int) getHidden
+{
+  int someivar = 3;
+  
+  return someivar;
+}
+@end
+
+int main(void)
+{
+  id object;
+
+  object = [[MyClass alloc] init];
+
+  /* Check for aliasing between instance variable and global
+     variable. */
+
+  if ([object getGlobal] != 1) {
+    abort();
+  }
+  
+  if ([object getInstance] != 2) {
+    abort();
+  }
+
+  /* Check whether the local variable hides the instance variable. */
+  
+  if ([object getHidden] != 3) {
+    abort();
+  }
+
+  return 0;
+}
Index: gcc/testsuite/objc.dg/shadow-1.m
===================================================================
--- gcc/testsuite/objc.dg/shadow-1.m	(revision 0)
+++ gcc/testsuite/objc.dg/shadow-1.m	(revision 0)
@@ -0,0 +1,33 @@
+/* Test disabling of warnings for shadowing instance variables.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do compile } */
+/* { dg-additional-options "-Wno-shadow-ivar" } */
+#include <objc/objc.h>
+
+@interface MyClass
+{
+@private
+  int private;
+
+@protected
+  int protected;
+
+@public
+  int public;
+}
+- (void) test;
+@end
+
+@implementation MyClass
+- (void) test
+{
+  int private = 12;
+  int protected = 12;
+  int public = 12;
+  int a;
+  
+  a = private;    /* No warning. */
+  a = protected;  /* No warning. */
+  a = public;     /* No warning. */
+}
+@end
Index: gcc/testsuite/objc.dg/ivar-visibility-1.m
===================================================================
--- gcc/testsuite/objc.dg/ivar-visibility-1.m	(revision 0)
+++ gcc/testsuite/objc.dg/ivar-visibility-1.m	(revision 0)
@@ -0,0 +1,33 @@
+/* Test instance variable visibility.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do compile } */
+#include <objc/objc.h>
+
+@interface MySuperClass
+{
+    int someivar;
+}
+@end
+
+@implementation MySuperClass
+@end
+
+
+@interface MyClass : MySuperClass 
+@end
+
+@implementation MyClass
+@end
+
+@interface MyOtherClass
+- (void) test: (MyClass *) object;
+@end
+
+@implementation MyOtherClass
+- (void) test: (MyClass *) object
+{
+  int a;
+
+  a = object->someivar;   /* { dg-error "instance variable .someivar. is declared protected" } */
+}
+@end
Index: gcc/testsuite/objc.dg/shadow-2.m
===================================================================
--- gcc/testsuite/objc.dg/shadow-2.m	(revision 0)
+++ gcc/testsuite/objc.dg/shadow-2.m	(revision 0)
@@ -0,0 +1,33 @@
+/* Test disabling of warnings for shadowing instance variables.  */
+/* Author: Dimitris Papavasiliou <dpapavas@gmail.com>.  */
+/* { dg-do compile } */
+/* { dg-additional-options "-Wno-shadow" } */
+#include <objc/objc.h>
+
+@interface MyClass
+{
+@private
+  int private;
+
+@protected
+  int protected;
+
+@public
+  int public;
+}
+- (void) test;
+@end
+
+@implementation MyClass
+- (void) test
+{
+  int private = 12;
+  int protected = 12;
+  int public = 12;
+  int a;
+  
+  a = private;    /* No warning. */
+  a = protected;  /* No warning. */
+  a = public;     /* No warning. */
+}
+@end