[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall closed this revision. rjmccall added a comment. Committed as r341489. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. In https://reviews.llvm.org/D44539#1208838, @rjmccall wrote: > Please read the developer policy: https://llvm.org/docs/DeveloperPolicy.html > > The information is on that page. > We grant commit access to contributors with a track record of submitting high > quality patches. I don't think I'm quite fitting these criteria yet :) Can you please commit this patch for me? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. Please read the developer policy: https://llvm.org/docs/DeveloperPolicy.html The information is on that page. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. > you can ask for commit privileges yourself. Ok, how do I do it? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. In https://reviews.llvm.org/D44539#1208780, @QF5690 wrote: > In https://reviews.llvm.org/D44539#1207982, @rjmccall wrote: > > > LGTM. > > > Thanks! What I should do next? Haven't found any info in docs about it :) https://llvm.org/docs/Contributing.html It's up to you. We can certainly commit it for you, but if you're likely to work on more patches, you can ask for commit privileges yourself. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. In https://reviews.llvm.org/D44539#1207982, @rjmccall wrote: > LGTM. Thanks! What I should do next? Haven't found any info in docs about it :) Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 updated this revision to Diff 161684.
QF5690 added a comment.
Using `isObjCARCImplicitlyUnretainedType` instead of `isObjCClassType`
Tests in `property-in-class-extension-1.m` relaxed to
`-Wproperty-attribute-mismatch` (was `-Weverything`)
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
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin11
-fobjc-runtime-has-weak -fobjc-weak -verify -Weverything %s
-// RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin11
-fobjc-runtime-has-weak -fobjc-weak -fsyntax-only -verify -Weverything %s
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin11
-fobjc-runtime-has-weak -fobjc-weak -verify -Wproperty-attribute-mismatch %s
+// RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin11
-fobjc-runtime-has-weak -fobjc-weak -fsyntax-only -verify
-Wproperty-attribute-mismatch %s
// rdar://12103400
@class NSString;
Index: test/SemaObjC/property-assign-on-object-type.m
===
--- /dev/null
+++ test/SemaObjC/property-assign-on-object-type.m
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wobjc-property-assign-on-object-type
%s
+
+@interface Foo @end
+@protocol Prot @end
+
+@interface Bar
+@property(assign, readonly) Foo* o1; // expected-warning {{'assign' property
of object type may become a dangling reference; consider using
'unsafe_unretained'}}
+@property(unsafe_unretained, readonly) Foo* o2;
+
+@property(assign) Class classProperty;
+@property(assign) Class classWithProtocolProperty;
+@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,14 @@
PropertyDecl->setInvalidDecl();
}
+ // Check for assign on object types.
+ if ((Attributes & ObjCDeclSpec::DQ_PR_assign) &&
+ !(Attributes & ObjCDeclSpec::DQ_PR_unsafe_unretained) &&
+ PropertyTy->isObjCRetainableType() &&
+ !PropertyTy->isObjCARCImplicitlyUnretainedType()) {
+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' property of object type may become a dangling reference; consider
using 'unsafe_unretained'">,
+ InGroup, 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
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fobjc-weak -verify -Weverything %s
-// RUN: %clang_cc1 -x objective-c++ -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fobjc-weak -fsyntax-only -verify -Weverything %s
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fobjc-weak -verify
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. Herald added a subscriber: jfb. Hey, still working on this? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added inline comments.
Comment at: test/SemaObjC/property-in-class-extension-1.m:18
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note
{{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //
expected-note {{property declared here}}
QF5690 wrote:
> rjmccall wrote:
> > Whoa, why is this test case using `-Weverything`? That seems unnecessarily
> > fragile.
> Do you think it should be relaxed only to warnings that are appearing here?
Yeah, tests should generally only be testing specific categories. Opt-out
warnings are different, of course, but it's understood that adding a new
opt-out warning is an ambitious change.
Repository:
rC Clang
https://reviews.llvm.org/D44539
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added inline comments.
Comment at: lib/Sema/SemaObjCProperty.cpp:2554
+ PropertyTy->isObjCRetainableType() &&
+ !PropertyTy->isObjCClassType()) {
+Diag(Loc, diag::warn_objc_property_assign_on_object);
rjmccall wrote:
> Please use `isObjCARCImplicitlyUnretainedType` here.
Thanks, didn't notice it.
Comment at: test/SemaObjC/property-in-class-extension-1.m:18
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note
{{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //
expected-note {{property declared here}}
rjmccall wrote:
> Whoa, why is this test case using `-Weverything`? That seems unnecessarily
> fragile.
Do you think it should be relaxed only to warnings that are appearing here?
Repository:
rC Clang
https://reviews.llvm.org/D44539
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added inline comments.
Comment at: lib/Sema/SemaObjCProperty.cpp:2554
+ PropertyTy->isObjCRetainableType() &&
+ !PropertyTy->isObjCClassType()) {
+Diag(Loc, diag::warn_objc_property_assign_on_object);
Please use `isObjCARCImplicitlyUnretainedType` here.
Comment at: test/SemaObjC/property-in-class-extension-1.m:18
-@property (assign, readonly) NSString* changeMemoryModel; // expected-note
{{property declared here}}
+@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //
expected-note {{property declared here}}
Whoa, why is this test case using `-Weverything`? That seems unnecessarily
fragile.
Repository:
rC Clang
https://reviews.llvm.org/D44539
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 updated this revision to Diff 150487.
QF5690 added a comment.
Remove warning for `Class` type, change warning message.
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,17 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wobjc-property-assign-on-object-type
%s
+
+@interface Foo @end
+
+@interface Bar
+@property(assign, readonly) Foo* o1; // expected-warning {{'assign' property
of object type may become a dangling reference; consider using
'unsafe_unretained'}}
+@property(unsafe_unretained, readonly) Foo* o2;
+
+@property(assign) Class classProperty;
+@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,14 @@
PropertyDecl->setInvalidDecl();
}
+ // Check for assign on object types.
+ if ((Attributes & ObjCDeclSpec::DQ_PR_assign) &&
+ !(Attributes & ObjCDeclSpec::DQ_PR_unsafe_unretained) &&
+ PropertyTy->isObjCRetainableType() &&
+ !PropertyTy->isObjCClassType()) {
+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' property of object type may become a dangling reference; consider
using 'unsafe_unretained'">,
+ InGroup, 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
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
aaron.ballman added a comment. In https://reviews.llvm.org/D44539#1106802, @rjmccall wrote: > In https://reviews.llvm.org/D44539#1106443, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D44539#1106152, @rjmccall wrote: > > > > > This was approved by the Objective-C language group as a default-off > > > warning. > > > > > > We usually do not expose new default-off warnings because experience shows > > that they rarely ever get enabled by users. If the ObjC group doesn't think > > this should be on by default, I wonder if it should be included in Clang at > > all. > > > That's a fair question to ask. In this case, I'm in favor of adding it > because we have evidence of there being a substantial set of users who would > enable it enthusiastically. Okay, that works for me. Thank you for verifying. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. In https://reviews.llvm.org/D44539#1106443, @aaron.ballman wrote: > In https://reviews.llvm.org/D44539#1106152, @rjmccall wrote: > > > This was approved by the Objective-C language group as a default-off > > warning. > > > We usually do not expose new default-off warnings because experience shows > that they rarely ever get enabled by users. If the ObjC group doesn't think > this should be on by default, I wonder if it should be included in Clang at > all. That's a fair question to ask. In this case, I'm in favor of adding it because we have evidence of there being a substantial set of users who would enable it enthusiastically. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1018 +def warn_objc_property_assign_on_object : Warning< + "'assign' attribute must not be of object type, use 'unsafe_unretained' instead">, + InGroup, DefaultIgnore; QF5690 wrote: > rjmccall wrote: > > "must" is rather strong for a warning. Maybe something more like "'assign' > > attribute on property of object type could be 'unsafe_unretained'"? > But "could be" is rather weak :) > May be "Prefer using explicit 'unsafe_unretained' attribute instead of > 'assign' for object types", or "Using explicit 'unsafe_unretained' attribute > instead of 'assign' for object types is preferred" (if passive voice is > preferred) Neither of those is quite in the standard diagnostic "voice". Maybe something like "'assign' property of object type may become a dangling reference; consider using 'unsafe_unretained'"? Oh, you should probably not warn about `Class` types. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1018 +def warn_objc_property_assign_on_object : Warning< + "'assign' attribute must not be of object type, use 'unsafe_unretained' instead">, + InGroup, DefaultIgnore; rjmccall wrote: > "must" is rather strong for a warning. Maybe something more like "'assign' > attribute on property of object type could be 'unsafe_unretained'"? But "could be" is rather weak :) May be "Prefer using explicit 'unsafe_unretained' attribute instead of 'assign' for object types", or "Using explicit 'unsafe_unretained' attribute instead of 'assign' for object types is preferred" (if passive voice is preferred) Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
aaron.ballman added a comment. In https://reviews.llvm.org/D44539#1106152, @rjmccall wrote: > This was approved by the Objective-C language group as a default-off warning. We usually do not expose new default-off warnings because experience shows that they rarely ever get enabled by users. If the ObjC group doesn't think this should be on by default, I wonder if it should be included in Clang at all. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. This was approved by the Objective-C language group as a default-off warning. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1018 +def warn_objc_property_assign_on_object : Warning< + "'assign' attribute must not be of object type, use 'unsafe_unretained' instead">, + InGroup, DefaultIgnore; "must" is rather strong for a warning. Maybe something more like "'assign' attribute on property of object type could be 'unsafe_unretained'"? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. This isn't really an Objective-C user forum, but I'll try to summarize briefly. `unsafe_unretained` is an unsafe version of `weak` — it's unsafe because it can be left dangling if the object is deallocated. It's necessary for a small (and getting smaller every year) set of classes that don't support true weak references, and it can be useful as an optimization to avoid the performance overhead of reference counting. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
Wizard added a comment. Is there any case for property of ObjC types that we should use `unsafe_unretained` or `assign` rather than `weak`? In my understanding, `weak` is for properties of ObjC types as the replacement of `unsafe_unretained` and `assign` is for properties of primitive types. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. It's waiting on a discussion that's happening pretty slowly, sorry. I know this is frustrating. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. It's been a couple of months now, @rjmccall any ETA's? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
rjmccall added a comment. We added the `unsafe_unretained` property attribute as part of ARC because we were introducing `__unsafe_retained` as a type qualifier and we wanted all the type qualifiers to have corresponding attribute spellings. `assign` is the much-older attribute, and its non-owning behavior was widely understood. In fact, we briefly considered naming the qualifier `__assign`, but we quickly decided that that we wanted a more explicit name for the ARC age. I like the idea of this warning, but I need to float it to our internal Objective-C language group before we can accept it. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
benhamilton added a comment. > Isn't it better to have unsafe_unretained there? That is a good question for the author of that property attribute. I'm not sure of the entire history. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. > I think the problem is there are valid places to use assign on object types > (especially delegates). Isn't it better to have unsafe_unretained there? I thought unsafe_unretained keyword is introduced specifically for that kinds of things. Ok, here is my last point :) Attributes like `strong`, `retain`, `copy` is commonly reffered as "ownership attribute". But `assign` does not tells anything about ownership, and from that point of view, it is semantically wrong to have `assign` on object types, and having `unsafe_unretained` that actually tells something about ownership – correct. If that's not the case, I can't understand the reason why `unsafe_unreatined` even exists. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
benhamilton added a comment. I think the problem is there are valid places to use assign on object types (especially delegates). There are a lot of special cases to deal with here, and it's really up to each team to decide the rules for when assign on object types are OK, so I don't think a compiler warning is appropriate. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added a comment. In https://reviews.llvm.org/D44539#1041993, @benhamilton wrote: > I wonder if this wouldn't be better as a clang-tidy check: > > https://github.com/llvm-mirror/clang-tools-extra/tree/master/clang-tidy/objc Border between clang-tidy checks and compiler diagnostics is not entirely clear for me, but, as I understand, clang-tidy is more about codestyle and some internal team agreements. The check that I'm adding is intended to prevent crashes, and it is much better to catch such things in compile time. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
aaron.ballman edited reviewers, added: rjmccall; removed: aaron.ballman. aaron.ballman added a subscriber: rjmccall. aaron.ballman added a comment. I don't know enough about ObjC to feel comfortable reviewing this, but I've added @rjmccall who may have opinions. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
benhamilton added a comment. I wonder if this wouldn't be better as a clang-tidy check: https://github.com/llvm-mirror/clang-tools-extra/tree/master/clang-tidy/objc Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
QF5690 added reviewers: aaron.ballman, benhamilton. QF5690 added a comment. Hey, saw many revisions around obj-c and sema that you are reviewing. Can you help me with this one? Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute
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, DefaultIgnore;
def warn_objc_property_no_assignment_attribute : Warning<
"no 'assign', 'retain', or 'copy' attribute is specified - "
"'assign' is assumed">,
Index:
