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

Swift 5 migration #48

Merged
merged 5 commits into from Apr 2, 2019
Merged

Swift 5 migration #48

merged 5 commits into from Apr 2, 2019

Conversation

GeroHerkenrath
Copy link
Contributor

It seems there wasn't much to do to migrate this to Swift 5, but since this is the first time I propose a PR to a foreign repo I am not sure whether I perhaps forgot something.

Besides the normal project file updates I would also propose this to increase the version of the pod to 4.1 as I am not sure it is fully backwards compatible. Thus I changed that in the podspec (together with specifying the swift version there instead of a .swift-version file).

@GeroHerkenrath
Copy link
Contributor Author

Well, I guess that's as far as I get for today. Seems like that needs to be fixed in Embassy first.

A quick google revealed that the problem with String(cString: UnsafePointer(strerror(errno))) can be relatively easy to fix: https://forums.developer.apple.com/thread/113919

This will probably also be relevant for envoy/Embassy#70, I might join in on that PR as well

@GeroHerkenrath
Copy link
Contributor Author

In case the thread in the Apple dev forums is lost, it should be noted that according to the answer there it would be possible to rewrite String(cString: UnsafePointer(strerror(errno))) as String(cString: strerror(errno)) since there is a specific overload in String to work with the c string directly.

@LFabien
Copy link

LFabien commented Apr 2, 2019

String(cString: UnsafePointer(strerror(errno))) was rewritten String(cString: strerror(errno)) here:
envoy/Embassy#66

Thanks for the PR @GeroHerkenrath !

@LFabien LFabien merged commit 4fe264a into envoy:master Apr 2, 2019
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.

None yet

2 participants