QF5690 created this revision.
QF5690 added a reviewer: rsmith.
QF5690 added a project: clang.
Herald added a subscriber: cfe-commits.
There is a problem, that `assign` attribute very often getting out of
attention. For example, consider this code:
@property(nonatomic, strong, readonly, nullable) SomeObjcClassType1* foo;
@property(nonatomic, strong, readwrite) SomeObjcClassType2* bar;
@property(nonatomic, assign, readonly) SomeObjcClassType* property;
@property(nonatomic, assign, readonly) SomeCStructType state;
It is very easy to miss that `assign` keyword, and it leads to hard to find and
reproduce bugs. Most of the time, we found such bugs in crash reports from
already in production code.
Now, consider this code:
@property(nonatomic, strong, readonly, nullable) SomeObjcClassType1* foo;
@property(nonatomic, strong, readwrite) SomeObjcClassType2* bar;
@property(nonatomic, unsafe_unretained, readonly) SomeObjcClassType* property;
@property(nonatomic, assign, readonly) SomeCStructType state;
It is now much harder to even make that mistake and it will be much obvious
during code review.
As there is no difference in behaviour between `assign` and `unsafe_unretained`
attribute, but second is much more verbose, saying "think twice when doing
this", I suggest to have, at least, optional warning, that will catch such
constructs.
This is my first revision in llvm, so any help would be very much appreciated.
Repository:
rC Clang
https://reviews.llvm.org/D44539
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaObjCProperty.cpp
test/SemaObjC/property-assign-on-object-type.m
test/SemaObjC/property-in-class-extension-1.m
Index: test/SemaObjC/property-in-class-extension-1.m
===================================================================
--- test/SemaObjC/property-in-class-extension-1.m
+++ test/SemaObjC/property-in-class-extension-1.m
@@ -15,12 +15,12 @@
@property (readonly) NSString* none;
@property (readonly) NSString* none1;
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note
{{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //
expected-note {{property declared here}}
@property (readonly) __weak id weak_prop;
@property (readonly) __weak id weak_prop1;
-@property (assign, readonly) NSString* assignProperty;
+@property (unsafe_unretained, readonly) NSString* unsafeUnretainedProperty;
@property (readonly) NSString* readonlyProp;
@@ -43,8 +43,8 @@
@property () __weak id weak_prop;
@property (readwrite) __weak id weak_prop1;
-@property (assign, readwrite) NSString* assignProperty;
-@property (assign) NSString* readonlyProp;
+@property (unsafe_unretained, readwrite) NSString* unsafeUnretainedProperty;
+@property (unsafe_unretained) NSString* readonlyProp;
@end
// rdar://12214070
Index: test/SemaObjC/property-assign-on-object-type.m
===================================================================
--- /dev/null
+++ test/SemaObjC/property-assign-on-object-type.m
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wobjc-property-assign-on-object-type
+
+@interface Foo @end
+
+@interface Bar
+@property(assign, readonly) Foo* o1; // expected-warning {{'assign' attribute
must not be of object type, use 'unsafe_unretained' instead}}
+@property(unsafe_unretained, readonly) Foo* o2;
+
+@property(assign) int s1;
+@property(assign) int* s2;
+@end
+
+@interface Bar ()
+@property(readwrite) Foo* o1;
+@property(readwrite) Foo* o2;
+@end
Index: lib/Sema/SemaObjCProperty.cpp
===================================================================
--- lib/Sema/SemaObjCProperty.cpp
+++ lib/Sema/SemaObjCProperty.cpp
@@ -2547,6 +2547,13 @@
PropertyDecl->setInvalidDecl();
}
+ // Check for assign on object types.
+ if ((Attributes & ObjCDeclSpec::DQ_PR_assign) &&
+ !(Attributes & ObjCDeclSpec::DQ_PR_unsafe_unretained) &&
+ PropertyTy->isObjCRetainableType()) {
+ Diag(Loc, diag::warn_objc_property_assign_on_object);
+ }
+
// Check for more than one of { assign, copy, retain }.
if (Attributes & ObjCDeclSpec::DQ_PR_assign) {
if (Attributes & ObjCDeclSpec::DQ_PR_copy) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -1014,6 +1014,9 @@
"property attributes '%0' and '%1' are mutually exclusive">;
def err_objc_property_requires_object : Error<
"property with '%0' attribute must be of object type">;
+def warn_objc_property_assign_on_object : Warning<
+ "'assign' attribute must not be of object type, use 'unsafe_unretained'
instead">,
+ InGroup<ObjCPropertyAssignOnObjectType>, DefaultIgnore;
def warn_objc_property_no_assignment_attribute : Warning<
"no 'assign', 'retain', or 'copy' attribute is specified - "
"'assign' is assumed">,
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -367,6 +367,7 @@
def BadFunctionCast : DiagGroup<"bad-function-cast">;
def ObjCPropertyImpl : DiagGroup<"objc-property-implementation">;
def ObjCPropertyNoAttribute : DiagGroup<"objc-property-no-attribute">;
+def ObjCPropertyAssignOnObjectType :
DiagGroup<"objc-property-assign-on-object-type">;
def ObjCProtocolQualifiers : DiagGroup<"objc-protocol-qualifiers">;
def ObjCMissingSuperCalls : DiagGroup<"objc-missing-super-calls">;
def ObjCDesignatedInit : DiagGroup<"objc-designated-initializers">;
Index: test/SemaObjC/property-in-class-extension-1.m
===================================================================
--- test/SemaObjC/property-in-class-extension-1.m
+++ test/SemaObjC/property-in-class-extension-1.m
@@ -15,12 +15,12 @@
@property (readonly) NSString* none;
@property (readonly) NSString* none1;
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note {{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; // expected-note {{property declared here}}
@property (readonly) __weak id weak_prop;
@property (readonly) __weak id weak_prop1;
-@property (assign, readonly) NSString* assignProperty;
+@property (unsafe_unretained, readonly) NSString* unsafeUnretainedProperty;
@property (readonly) NSString* readonlyProp;
@@ -43,8 +43,8 @@
@property () __weak id weak_prop;
@property (readwrite) __weak id weak_prop1;
-@property (assign, readwrite) NSString* assignProperty;
-@property (assign) NSString* readonlyProp;
+@property (unsafe_unretained, readwrite) NSString* unsafeUnretainedProperty;
+@property (unsafe_unretained) NSString* readonlyProp;
@end
// rdar://12214070
Index: test/SemaObjC/property-assign-on-object-type.m
===================================================================
--- /dev/null
+++ test/SemaObjC/property-assign-on-object-type.m
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wobjc-property-assign-on-object-type
+
+@interface Foo @end
+
+@interface Bar
+@property(assign, readonly) Foo* o1; // expected-warning {{'assign' attribute must not be of object type, use 'unsafe_unretained' instead}}
+@property(unsafe_unretained, readonly) Foo* o2;
+
+@property(assign) int s1;
+@property(assign) int* s2;
+@end
+
+@interface Bar ()
+@property(readwrite) Foo* o1;
+@property(readwrite) Foo* o2;
+@end
Index: lib/Sema/SemaObjCProperty.cpp
===================================================================
--- lib/Sema/SemaObjCProperty.cpp
+++ lib/Sema/SemaObjCProperty.cpp
@@ -2547,6 +2547,13 @@
PropertyDecl->setInvalidDecl();
}
+ // Check for assign on object types.
+ if ((Attributes & ObjCDeclSpec::DQ_PR_assign) &&
+ !(Attributes & ObjCDeclSpec::DQ_PR_unsafe_unretained) &&
+ PropertyTy->isObjCRetainableType()) {
+ Diag(Loc, diag::warn_objc_property_assign_on_object);
+ }
+
// Check for more than one of { assign, copy, retain }.
if (Attributes & ObjCDeclSpec::DQ_PR_assign) {
if (Attributes & ObjCDeclSpec::DQ_PR_copy) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -1014,6 +1014,9 @@
"property attributes '%0' and '%1' are mutually exclusive">;
def err_objc_property_requires_object : Error<
"property with '%0' attribute must be of object type">;
+def warn_objc_property_assign_on_object : Warning<
+ "'assign' attribute must not be of object type, use 'unsafe_unretained' instead">,
+ InGroup<ObjCPropertyAssignOnObjectType>, DefaultIgnore;
def warn_objc_property_no_assignment_attribute : Warning<
"no 'assign', 'retain', or 'copy' attribute is specified - "
"'assign' is assumed">,
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -367,6 +367,7 @@
def BadFunctionCast : DiagGroup<"bad-function-cast">;
def ObjCPropertyImpl : DiagGroup<"objc-property-implementation">;
def ObjCPropertyNoAttribute : DiagGroup<"objc-property-no-attribute">;
+def ObjCPropertyAssignOnObjectType : DiagGroup<"objc-property-assign-on-object-type">;
def ObjCProtocolQualifiers : DiagGroup<"objc-protocol-qualifiers">;
def ObjCMissingSuperCalls : DiagGroup<"objc-missing-super-calls">;
def ObjCDesignatedInit : DiagGroup<"objc-designated-initializers">;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
