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: 1888 bytes --]

On 04/25/2014 04:07 AM, Mike Stump wrote:
> On Apr 24, 2014, at 4:09 PM, Dimitris Papavasiliou<dpapavas@gmail.com>  wrote:
>> On 04/24/2014 07:00 PM, Mike Stump wrote:
>>> On Feb 6, 2014, at 1:25 AM, Dimitris Papavasiliou<dpapavas@gmail.com>  wrote:
>>>> This is a patch regarding a couple of Objective-C related dialect options and warning switches.
>>>
>>> Ok.
>>>
>>> Committed revision 209753.
>>>
>>> If you could, please add documentation and a test case.
>>
>> Thanks for taking the time to look at this, although to be honest I didn't expect a straight merge into the trunk.
>
> DonÂ’t submit changes you donÂ’t want!  :-)
>
>> I'll add documentation and test cases as soon as I figure out how.
>
> Just copy testsuite/objc.dg/private-2.m into shadow-1.m and then `fixÂ’ it to test what you want.  If you need one with and one without a flag, copy it twice and use something like:
>
> // { dg-options "-Wshadow" }
>
> on it.  Type make RUNTESTFLAGS=dg.exp=shadow-1.m check-objc to test it.
>
> For the doc, just find a simple option in the part of the manual you want to put it in, and copy it.  We document the non-default, (Wno-shadow-ivar for example) options.

I'm attaching three additional patches:

documentation.patch:  This adds documentation for the three additional 
switches plus a small change to reflect the fact that -Wshadow now 
controls instance variable shadowing as well.

tests.patch:  This adds 9 test cases that test both the intended 
behavior of the added switches as well as the default behavior in their 
absence.

enabledby_wshadow.patch:  This adds the proposed change to allow 
specifying -Wno-shadow to turn off -Wshadow-ivar as well plus one more 
test to make sure that it happens.

I've run make check-objc with all these changes and everything seems to 
be fine.  Let me know if you need anything else and thanks again for 
your time.

Dimitris

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

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 209787)
+++ 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  -Wshadow-ivar @gol
 -Wsign-compare  -Wsign-conversion -Wfloat-conversion @gol
 -Wsizeof-pointer-memaccess @gol
 -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol
@@ -2975,6 +2977,21 @@
 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
+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
@@ -4349,11 +4366,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 -Wshadow-ivar @r{(Objective-C only)}
+@opindex Wshadow-ivar
+@opindex Wno-shadow-ivar
+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}

[-- Attachment #3: enabledby_wshadow.patch --]
[-- Type: text/x-patch, Size: 1417 bytes --]

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;    /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
+  a = protected;  /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
+  a = public;     /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
+}
+@end
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

[-- Attachment #4: tests.patch --]
[-- Type: text/x-patch, Size: 8817 bytes --]

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;    /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
+  a = protected;  /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
+  a = public;     /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
+}
+@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,21 @@
+/* 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;
+  
+  a = self->someivar;  /* Ok */
+  a = someivar;        /* { dg-error ".someivar. undeclared" "" { xfail *-*-* } } */
+}
+@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;  /* Ok */
+  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;        /* { dg-warning "hides instance variable" "" { xfail *-*-* } } */
+}
+@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,86 @@
+/* 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;
+
+@interface MyClass
+{
+  Class isa;
+  int someivar;
+}
+
++ (id) new;
++ (id) alloc;
+- (id) init;
+- (int) getGlobal;
+- (int) getInstance;
+- (int) getHidden;
+@end
+
+@implementation MyClass
++ (id) new
+{
+  return [[self alloc] init];
+}
+
++ (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 new];
+
+  /* Check for aliasing between instance variable and global
+     variable. */
+
+  if ([object getGlobal] != 1) {
+    abort();
+  }
+  
+  if ([object getInstance] != 2) {
+    abort();
+  }
+
+  /* Check wheter the local variable hides the instance variable. */
+  
+  if ([object getHidden] != 3) {
+    abort();
+  }
+
+  return 0;
+}
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/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-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-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,34 @@
+/* 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;
+
+  a = object->someivar;   /* { dg-error "instance variable .someivar. is declared (private|protected)" "" { xfail *-*-* } } */
+}
+@end