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.

Sign up for GitHub

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

Jump to bottom

ReactiveObjC: upgrade deployment target to 12.0 and eliminate Xcode12 warnings. #40

Merged
merged 10 commits into from
Aug 27, 2020
Merged

ReactiveObjC: upgrade deployment target to 12.0 and eliminate Xcode12 warnings. #40

merged 10 commits into from
Aug 27, 2020

Conversation

arangato
Copy link
Member

@arangato arangato commented Aug 18, 2020 •
edited
Loading

Here is a brief summary:

  • Replace deprecated OSAtomic APIs with stdatomic.
  • Use latest Nimble and Quick libraries.
  • Raise deployment target to 12.0
  • Enable recommended warning for quoited framework includes
    • Disabled it for tests because there are several of these in Nimble and Quick
  • Use Xcode 12 in circle CI.

See commit messages for more details.

@arangato arangato force-pushed the feature/xcode12 branch 9 times, most recently from 6f6c829 to 8e90409 Compare August 19, 2020 10:24
@arangato arangato changed the title [ci test]Feature/xcode12 ReactiveObjC: upgrade deployment target to 12.0 and eliminate Xcode12 warnings. Aug 19, 2020
@arangato arangato marked this pull request as ready for review August 19, 2020 10:31
@arangato
Copy link
Member Author

@barakwei this one is ready for review.

@barakwei barakwei self-assigned this Aug 20, 2020
@barakwei
Copy link
Member

@StatusReport @barakyoresh This PR (among other things) replaces OSAtomic API with stdatomic. The code is LGTM on my end. Do you wish to go over as well?
The reference PR from upstream: ReactiveCocoa#178

StatusReport
StatusReport reviewed Aug 23, 2020
View reviewed changes
Copy link
Member

@StatusReport StatusReport left a comment

Choose a reason for hiding this comment

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

Overall LGTM, up to what I can say about atomic operations in a limited time review.

Please also run all the tests with TSan to see that it doesn't complain on anything after this migration.

ReactiveObjC/RACCommand.m Outdated Show resolved Hide resolved
ReactiveObjC/RACDynamicSequence.m Show resolved Hide resolved
@arangato
Copy link
Member Author

Tests with TSan pass as well. No issues spotted.
There is a PR#179. upstream that replaces ReactiveCocoa#178, but it's the same thing squashed.

barakwei
barakwei reviewed Aug 27, 2020
View reviewed changes
ReactiveObjC/RACCommand.m Outdated Show resolved Hide resolved
@arangato
Copy link
Member Author

done.

@barakwei barakwei merged commit 3bdc11b into Lightricks:master Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StatusReport StatusReport StatusReport left review comments

@barakwei barakwei barakwei approved these changes

Assignees

@barakwei barakwei

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arangato @barakwei @StatusReport