Remove dependency on Half - Githubissues

SomeRandomiOSDev / CBORCoding

Easy CBOR encoding and decoding for iOS, macOS, tvOS and watchOS.

MIT License

49 stars 14 forks source link

Remove dependency on Half #21

Closed christophhagen closed 12 months ago

christophhagen commented 1 year ago

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:

[x] Code compilation.
[x] All tests passing.
[x] 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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@christophhagen OK, fixing it for x64 macs was easy: https://github.com/christophhagen/CBORCoding/pull/1

dabrahams commented 1 year ago

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

SomeRandomiOSDev commented 12 months ago

@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 commented 12 months ago

Closing the PR due to #27.