Skip to content

Add @_objcImplementation attribute#798

Merged
beccadax merged 1 commit intoswiftlang:mainfrom
beccadax:at-implementation
Oct 21, 2022
Merged

Add @_objcImplementation attribute#798
beccadax merged 1 commit intoswiftlang:mainfrom
beccadax:at-implementation

Conversation

@beccadax
Copy link
Copy Markdown
Contributor

@beccadax beccadax commented Sep 15, 2022 •
edited
Loading

Part of swiftlang/swift#60630, but safe to merge before the compiler-side implementation.

@beccadax beccadax requested a review from CodaFi September 15, 2022 22:24
@beccadax beccadax requested a review from ahoppen as a code owner September 15, 2022 22:24
@beccadax
Copy link
Copy Markdown
Contributor Author

beccadax commented Sep 15, 2022

@swift-ci please test

@beccadax
Copy link
Copy Markdown
Contributor Author

beccadax commented Sep 15, 2022

@CodaFi I'd like your opinion about the test I added in this PR. Is it worth including given that @_objcImplementation is just using the generalized attribute parsing path, rather than something custom?

@beccadax
Copy link
Copy Markdown
Contributor Author

beccadax commented Sep 15, 2022

With swiftlang/swift#60630

@swift-ci please test

CodaFi
CodaFi approved these changes Sep 16, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

LGTM, test especially. Thanks for adding it.

@CodaFi
Copy link
Copy Markdown
Contributor

CodaFi commented Sep 16, 2022

Since you're not updating the C++ libSyntax parsing this can be merged independently of the compiler PR whenever you want.

@beccadax
Copy link
Copy Markdown
Contributor Author

beccadax commented Sep 16, 2022

@CodaFi You’d think so, but it actually breaks the compiler build by introducing references to an ObjCImplementationAttr class that hasn’t been added there yet. Check it out: https://ci.swift.org/job/swift-syntax-PR-macOS/615/consoleText

@beccadax beccadax force-pushed the at-implementation branch 2 times, most recently from a71cb0d to 2688154 Compare October 5, 2022 17:40
@beccadax beccadax mentioned this pull request Oct 5, 2022
Merged
ahoppen
ahoppen reviewed Oct 6, 2022
View reviewed changes
gyb_syntax_support/AttributeKinds.py
NotSerialized,
ABIBreakingToAdd, ABIBreakingToRemove, APIBreakingToAdd, APIBreakingToRemove,
code=71),
# NOTE: 72 is unused
Copy link
Copy Markdown
Member

@ahoppen ahoppen Oct 6, 2022

Choose a reason for hiding this comment

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

I have a vague memory of reading somewhere that it’s unsafe to reuse old codes but can’t find it anymore. @CodaFi do you remember readings something of the sort when migrating AttributeKinds.py from the compiler repository?

@beccadax
Add @_objcImplementation attribute
ae622f9 
Part of swiftlang/swift#60630, but safe to merge before the compiler-side implementation.
@beccadax
Copy link
Copy Markdown
Contributor Author

beccadax commented Oct 20, 2022

With swiftlang/swift#60630

@swift-ci please test

@beccadax beccadax merged commit 397798a into swiftlang:main Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahoppen ahoppen ahoppen left review comments

@CodaFi CodaFi CodaFi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@beccadax @CodaFi @ahoppen