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

Update to CRoaring v4.2.1 for #21 #22

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

wti
Copy link

@wti wti commented Feb 19, 2025

Update to CRoaring release v4.2.1 for #21

  • Aggregated .c and .h files copied from release (first commit)
  • Migrated .c off two deprecated API (see below)
  • Minor test additions to improve coverage and minimize or bracket print output
  • README updated to remove unsupported code (now manually verified)

re: Swift interface:

  • Swift 4 is not really supported any more.
  • The Swift interface includes inlined functions which don't appear in coverage reports, but tests hit all others. (Coverage of CRoaring is marginal but not at issue here.)

re: CRoaring

  • Fixing the two deprecated API's creates a delta with the release code. Let me know if you want instead to leave those as-is.
  • There are ~40 warnings about implicit casts. I started to address some in a separate branch, but I stopped because I'm not the one to validate their correctness and deltas are unwelcome.

@lemire
Copy link
Member

lemire commented Feb 19, 2025

Running tests.

@wti
Copy link
Author

wti commented Feb 20, 2025

test-macOS.yml is passing, but test-linux.yml has header conflicts.

@wti wti marked this pull request as draft February 20, 2025 01:40
@wti
Copy link
Author

wti commented Feb 20, 2025

Converted PR to DRAFT. I'll stop investigating for now.

  • macOS CI is working
  • Linux CI fails to build SwiftRoaring's CRoaring module, even on ubuntu-latest (where CRoaring project CI passes).
  • It builds/tests fine on my local stock Ubuntu 24.04 with Swift 6.0.3.
  • Unclear if the issue is extrinsic library/build configuration/setup or true problems with amalgamated intrinsics header inclusion in this context.

Let me know if instead you'd like to merge and possibly release, deferring CI fixes.

Sorry for the noise+hassle of an incomplete PR.

@lemire
Copy link
Member

lemire commented Feb 20, 2025

I think we ought to figure out the issue prior to merging.

Do you understand the error message? Something to do with intrinsics... ?

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.

2 participants