Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow @objc(Name) on enums #618

Merged
merged 3 commits into from
Dec 24, 2015
Merged

Conversation

lilyball
Copy link
Contributor

This implements support for

@objc(ObjCEnum) enum SwiftEnum : Int {
    case One = 1, Two, Three
}

A new macro for the generated header is defined called SWIFT_ENUM_NAMED and the above declaration looks like

typedef SWIFT_ENUM_NAMED(NSInteger, ObjCEnum, "SwiftEnum") {
    ObjCEnumOne = 1,
    ObjCEnumTwo = 2,
    ObjCEnumThree = 3,
};

Since swift_name doesn't actually work properly yet when importing Obj-C enums, there's an XFAILed test that tests the clang importer here.

This does NOT cover support for @objc(Name) on the individual case declarations.

// FIXME: @objc(Renamed) enums should work
// XFAIL: *

// RUN: %target-swift-frontend -emit-sil %s -import-objc-header %S/Inputs/enum-objc.h -verify
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if this sort of test is the right way to do this, but I was using the test/ClangModules/enum-new.swift test as a prototype.

@jrose-apple
Copy link
Contributor

Thanks for working on this!

@@ -0,0 +1,12 @@
// FIXME: @objc(Renamed) enums should work
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, you should be able to add the flag -enable-swift-name-lookup-tables to this test to get it working now. The Swift name lookup tables aren't on by default just yet, but they address this problem already.

@lilyball
Copy link
Contributor Author

I've updated the PR as per feedback. Tests are now part of the relevant commits, the enum constant prefix bit was pulled out into #743, and I've added relevant code to the resolve-cross-language test.

A couple of the tests have // FIXME comments for #743, because they use __attribute__((swift_name(...))) on enum constants in order to sidestep the enum constant prefix stripping. The tests will be unaffected by #743 but should ideally have the attributes removed once #743 lands.

@jrose-apple
Copy link
Contributor

jrose-apple commented Dec 23, 2015

Looks great. Thank you!

I merged #743 just now. Do you want to fix these tests first or just do a follow-up?

@lilyball
Copy link
Contributor Author

I'll fix the tests now.

@lilyball
Copy link
Contributor Author

@jrose-apple Rebased and updated to remove the FIXMEs.

@jrose-apple
Copy link
Contributor

Aren't these commits in the wrong order? Ideally every commit should be build-and-testable on its own, although I suppose that matters less in a merge.

@lilyball
Copy link
Contributor Author

@jrose-apple GitHub bug, it's showing the commits in the wrong order. If you look at the branch they're ordered Sema, PrintObjC, ClangImporter. This bug can occur when rewriting commits, because it's sorting based on AuthorDate instead of actual topographical order or CommitDate.

@jrose-apple
Copy link
Contributor

Weird, okay. Doing one more check on my machine, then I'll merge.

("It's not that I don't trust you…")

@lilyball
Copy link
Contributor Author

Yeah, it's annoying. I filed a support ticket over a year ago about this, and they still haven't fixed it :/

jrose-apple added a commit that referenced this pull request Dec 24, 2015
Allow @objc(Name) on enums.

rdar://problem/21930334
@jrose-apple jrose-apple merged commit 8c839b6 into swiftlang:master Dec 24, 2015
@lilyball lilyball deleted the enum-objc-name branch December 24, 2015 02:30
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
[DO NOT MERGE] Update swift-argument-parser
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants