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
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
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
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
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
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
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
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
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
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; //
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
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
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
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
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
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;
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
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
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)
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.
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
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
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
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
___
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
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
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
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
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
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
___
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,
31 matches
Mail list logo
