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

Use objc2-core-foundation #147

Closed
wants to merge 2 commits into from

Conversation

madsmtm
Copy link

@madsmtm madsmtm commented Jan 27, 2025

objc2-core-foundation is a (mostly) auto-generated version of core-foundation-sys, with two important differences:

  1. It handles memory management internally (CFRetain/CFRelease).
  2. It contains helpers for doing the string conversion.
    In particular, CFStringGetCStringPtr is actually incorrect to use if the string is not UTF-8 internally, see this test (probably not that relevant, since timezones are always gonna be ASCII strings, but still).

Both of these are certainly possible to handle manually (as you have done), but they add up to unnecessary mental overhead.

Implementing #145 with CFNotificationCenter, if you ever wanted to do that, should also be easier with objc2-core-foundation.

This bumps MSRV on Apple platforms to 1.71 (for extern "C-unwind", while CoreFoundation probably won't unwind by itself, it calls out to a lot of things that could), so you might want to wait with this for a while.

Builds upon #146.

@lopopolo
Copy link
Collaborator

I am hesitant to swap out dependencies without a super compelling reason to do so, especially since core-foundation-sys is stable, maintained by the Servo team, and has zero dependencies. As a low level crate in most project's lockfiles, I'd like to think we try our best to minimize dependency weight (this is something we did when moving from cxx to cc when adding platform support for Haiku).

Re this comment in the issue:

In particular, CFStringGetCStringPtr is actually incorrect to use if the string is not UTF-8 internally, see this test (probably not that relevant, since timezones are always gonna be ASCII strings, but still).

The test you link shows unexpected behavior of CFStringGetCString (not the Ptr func). CFStringGetCStringPtr (the function we do use) is quite clearly documented to return NULL if the internal representation of the CFString is not compatible with the requested encoding. The safety docs hint at this, but don't call it out explicitly; we should make that more clear.

Re: the Notification Center bits, it is not an accepted feature and I'd rather not make dep changes preemptively to support a feature we may not accept.

I'm not sure of our current MSRV policy, it previously bumping one Tier1 platform and not the others isn't something we'd entertain due to our reverse dependencies.

Copy link
Collaborator

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

I'm not in favor of merging this, @Kijewski @astraw thoughts?

@lopopolo lopopolo added dependencies Pull requests that update a dependency file Tier-1 Rust Tier-1 platform Tier-3 Rust Tier-3 platform labels Jan 28, 2025
@madsmtm
Copy link
Author

madsmtm commented Jan 28, 2025

Totally fine, thanks for the consideration! I'll just close this myself, but feel free to re-open if you re-consider.

is quite clearly documented to return NULL if the internal representation of the CFString is not compatible with the requested encoding.

It is documented that way, but it doesn't actually do this, at least not from my investigation. A more concise example in C:

#include <CoreFoundation/CoreFoundation.h>

int main() {
    // Supposed to contain a "♥".
    char* heart_unicode = "\x65\x26";
    char* heart_utf8 = "\xE2\x99\xA5";

    // Create CFString with the heart from unicode.
    CFStringRef s = CFStringCreateWithCString(kCFAllocatorDefault, heart_unicode, kCFStringEncodingUnicode);

    // CFStringGetCString converts to UTF-8.
    char buf[20];
    CFStringGetCString(s, (char*)&buf, 20, kCFStringEncodingUTF8);
    printf("%s, %d, %d\n", buf, strcmp(buf, heart_unicode), strcmp(buf, heart_utf8)); // prints "♥, 125, 0"

    // But CFStringGetCStringPtr completely ignores the UTF-8 conversion
    // we asked it to do, i.e. a huge correctness footgun!
    const char* ptr = CFStringGetCStringPtr(s, kCFStringEncodingUTF8);
    printf("%s, %d, %d\n", ptr, strcmp(ptr, heart_unicode), strcmp(ptr, heart_utf8)); // prints "e&, 0, -125"
}

But regardless, this is overly pedantic of me, and probably works for your use-case (instead, I should file a documentation bug report with Apple on this EDIT: swiftlang/swift-corelibs-foundation#5164).

@madsmtm madsmtm closed this Jan 28, 2025
@madsmtm
Copy link
Author

madsmtm commented Jan 30, 2025

The test you link shows unexpected behavior of CFStringGetCString

Thanks again for this comment btw, I didn't understand until now that I was indeed wrong here, sorry for the confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Tier-1 Rust Tier-1 platform Tier-3 Rust Tier-3 platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants