This is an archive of the discontinued LLVM Phabricator instance.

[objc_direct] Small updates to help with adoption.
ClosedPublic

Authored by MadCoder on Jan 30 2020, 5:20 PM.

Details

Summary

Add fixits for messaging self in MRR or using super, as the intent is
clear, and it turns out people do that a lot more than expected.

Allow for objc_direct_members on main interfaces, it's extremely useful
for internal only classes, and proves to be quite annoying for adoption.

Add some better warnings around properties direct/non-direct clashes (it
was done for methods but properties were a miss).

Add some errors when direct properties are marked @dynamic.

Radar-Id: rdar://problem/58355212

Diff Detail

Event Timeline

MadCoder created this revision.Jan 30 2020, 5:20 PM
MadCoder updated this revision to Diff 243649.Feb 10 2020, 12:20 PM
MadCoder edited the summary of this revision. (Show Details)
Add some errors when direct properties are marked @dynamic.
clang/include/clang/Basic/DiagnosticSemaKinds.td
1022–1024

This diagnostics reads a bit awkward when a property is conflicting with a property, since the the first half of the diagnostic is talking about the property declaration, but the second half is implicitly referring to the generated getter/setter.

1040

I think this might be more clear as: "direct property cannot be @dynamic"

clang/lib/Sema/SemaExprObjC.cpp
3032

I believe its UB to call a destructor twice. Please use a compound statement to control where the dtor will be called here.

3048

ditto

clang/lib/Sema/SemaObjCProperty.cpp
1630–1637

Would you mind adding a test for this diagnostic? It looks like err_objc_direct_dynamic_property doesn't take a parameter too, so there isn't any reason to do << PropertyId unless you add one.

2436

Just to be clear: so we need this check because the getter/setters of a property declared in a category aren't considered to override any getters/setters of the extended class, so the existing CheckObjCMethodOverrides checks below don't work?

clang/test/SemaObjC/method-direct.m
51

Can you leave this in so we have a test that has this attribute on an @interface?

plotfi added a subscriber: plotfi.Feb 13 2020, 11:30 AM

@MadCoder Could there be a way to mark objc_direct methods as something like attribute((visibility("default"))) or __declspec(dllexport) to denote that a given method should be directly callable but still retain the metadata to be called through an objc_msgSend from an external dylib? Could that help to boost adoption?

@MadCoder Could there be a way to mark objc_direct methods as something like attribute((visibility("default"))) or __declspec(dllexport) to denote that a given method should be directly callable but still retain the metadata to be called through an objc_msgSend from an external dylib? Could that help to boost adoption?

I'm not sure how that is relevant to this review. secondly this kind of defeats the entire point of objc_direct which is to _not_ generate all that metadata.

MadCoder updated this revision to Diff 244773.Feb 14 2020, 2:18 PM
MadCoder marked 10 inline comments as done.

@erik.pilkington review feedback

clang/lib/Sema/SemaObjCProperty.cpp
1630–1637

there were some, I forgot to add the test case to the commit ...

2436

not quite, it's because people do things like this:

@interface Foo (Cat)
@property int bar;
@end

But implement the property in the main @implementation (or the other way around) and this is not considered as overrides today, or even worse, many many times the Category doesn't even have a corresponding @implementation anywhere. Tthere's one direction of this that has a possible optional warning though today in the compiler.

But direct method resolution requires to not cross streams else the incorrect symbol is formed (because the category is an actual different namespace). With dynamism it actually doesn't matter which is why the compiler doesn't care today.

erik.pilkington accepted this revision.Feb 14 2020, 3:47 PM

LGTM, thanks.

clang/lib/Sema/SemaObjCProperty.cpp
2436

Okay, thanks for clarifying.

This revision is now accepted and ready to land.Feb 14 2020, 3:47 PM
This revision was automatically updated to reflect the committed changes.