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

Remove dependency on Half #21

Closed
wants to merge 2 commits into from

Conversation

christophhagen
Copy link

Description

This PR removes the dependency on Half, which is currently used to provide a half-precision floating point type. Float16 was introduced with Swift 5.3, so the dependency is no longer needed.

Checklist

Ensure that your pull request has followed all the steps below:

  • Code compilation.
  • All tests passing.
  • No new SwiftLint issues.
  • Added new unit tests, if applicable.
  • Extended the documentation (including README), if applicable.
  • Updated version in CBORCoding.podspec following semver guidelines.
  • Ran workflowtests.sh and passed.
  • Added myself to the CONTRIBUTORS file.

Proposed changes

The main changes are:

  • Remove the dependency on Half
  • Use the native Float16 type instead

Minor changes are:

  • Minor restructuring of large if-else statements
  • Fixed some linting warnings

Additional Info

The dependency on Float16 requires higher minimum deployment targets on all platforms:

  • iOS: 9.0 -> 14.0
  • macOS: 10.10 -> 11.0
  • tvOS: 9.0 -> 14.0
  • watchOS: 2.0 -> 7.0

For systems with swift version lower than 5.3, the dependency on Half is still needed. In this case the typealias Float16 = Half is included. The package descriptions for 4.2 and 5.0 still include the dependency. The correct compilation on older platforms is untested.

@dabrahams
Copy link
Contributor

dabrahams commented Nov 9, 2023

@SomeRandomiOSDev what's it going to take to get this PR accepted? The failed "@codacy-production
Codacy Static Code Analysis" gives no clues. Oh, @christophhagen, I see the answer is easy: bring back Package.resolved, but use the one generated on your machine after you swift package resolve, that no longer mentions Half. Checking it in is best practice anyway.

With these changes, the library passes all tests on Windows and I would submit CI actions to test there.

@dabrahams
Copy link
Contributor

dabrahams commented Nov 9, 2023

Argh, for my purposes this is not a fully-complete solution because Float16 is apparently only available on Apple Silicon (seriously, Apple?)

I wonder if support for CBOR serializing 16-bit floats really needs to be built into the core of this library, or if it could be added as an extension the way we normally would with any user-defined types?

@dabrahams
Copy link
Contributor

@christophhagen I took a first cut at making it work on x64 macs, but it's clear that some of the old logic needs to be restored because the build is failing. The CI job passes for some reason, which also needs to be corrected in the workflow file.

@dabrahams
Copy link
Contributor

@christophhagen OK, fixing it for x64 macs was easy: christophhagen#1

@dabrahams
Copy link
Contributor

@christophhagen And here is the PR that ensures failures in CI are actually reported.

@SomeRandomiOSDev
Copy link
Owner

@christophhagen I created a release to address a lot of the issues that came up due to this repo being so out of date. Please rebase on this release and update this PR accordingly

@christophhagen
Copy link
Author

Closing the PR due to #27.

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