Skip to content

/ core-foundation-rs

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

CFString refactor / cleanup #76

Closed
wants to merge 2 commits into from
Closed

CFString refactor / cleanup #76

wants to merge 2 commits into from
+17 −10

Conversation

@TimNN
Copy link
Contributor

TimNN commented Dec 18, 2015

This pull request:

  • removes two unnecessary casts (from CFIndex to usize and back again)
  • extracts the to rust string conversion: instead of doing it on-the-fly in the Display implementation (and thus only supporting the indirect ToString way to get a rust string from a CFString, which always allocates) this implements From<&CFString> for Cow<str> allowing everyone the possibility of avoiding an allocation if the CFString is already formatted in UTF-8.

Review on Reviewable
TimNN added 2 commits Dec 18, 2015
@TimNN
CFString: extract to Cow conversion
8f6771a
@TimNN
CFString: remove unnecessary casts
Loading status checks…
f5c07a8
@metajack
Copy link
Contributor

metajack commented Apr 19, 2016

r? @pcwalton
@nox
Copy link
Member

nox commented Sep 1, 2016

Isn't CFString a UTF-16 thing?
@TimNN
Copy link
Contributor Author

TimNN commented Sep 1, 2016

As far as I know, CFString does not have a specific encoding. So if it's already UTF-8, we can get away without allocating, otherwise we need a buffer so the string can be encoded as UTF-8 into it.
@nox
Copy link
Member

nox commented Sep 1, 2016 •
edited

The CFStringGetLength function returns the total number (in terms of UTF-16 code pairs) of characters in the string.

https://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFStringRef/
@TimNN
Copy link
Contributor Author

TimNN commented Sep 1, 2016

Yes, but as far as I understand, this function is only used to verify that all characters in the string have been written.

All functions actually accessing the string data always specify kCFStringEncodingUTF8.
@nox
Copy link
Member

nox commented Sep 1, 2016

Oh I see. Could we use CFStringGetCString in the Owned branch?
@TimNN
Copy link
Contributor Author

TimNN commented Sep 1, 2016

We probably could, but from reading the documentation I think CFStringGetBytes is more appropriate: We don't care about the NUL terminator which CFStringGetCString appends and CFStringGetBytes can also tell us how many bytes we actually need in the output buffer (which CFStringGetCString cannot as far as I can tell).
@nox
Copy link
Member

nox commented Sep 1, 2016

Oh right.
jdm pushed a commit that referenced this pull request Feb 1, 2018
@brson
Merge pull request #76 from simnalamburt/master
04a0ba5 
Fix build error
jdm pushed a commit that referenced this pull request Feb 1, 2018
@jrmuizel
Merge pull request #76 from jrmuizel/callback-types
4a47f13 
Fix callback types
jdm pushed a commit that referenced this pull request Feb 1, 2018
@bors-servo
Auto merge of #76 - poiru:fix-family-name, r=jdm
2348464 
Make CTFontDescriptor::family_name() actually return the family name

It was incorrectly returning the display name, which already has a separate
function.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-text-rs/76)
<!-- Reviewable:end -->
@TimNN TimNN closed this May 28, 2018
@jdm
Copy link
Member

jdm commented May 28, 2018

@TimNN Is there any reason not to merge these changes besides maintainers being unresponsive?
@TimNN
Copy link
Contributor Author

TimNN commented May 28, 2018

So I thought that there had been a rather large change to the string module that would obsolete / complicate the changes proposed here, but apparently that is not the case. I'm no longer working on the (hobby) project which needed these changes and was cleaning up my open PRs.

If there's still interest in getting this merged, I'd be happy to rebase the PR or let someone else take over.
@jdm
Copy link
Member

jdm commented May 29, 2018

I'm in favour of merging a rebased version :)
@TimNN TimNN mentioned this pull request May 29, 2018
Merged
@TimNN
Copy link
Contributor Author

TimNN commented May 29, 2018

Opened #220 with the rebase.
bors-servo added a commit that referenced this pull request May 29, 2018
@bors-servo
Auto merge of #220 - TimNN:cfstring-refactor, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
2aac8fb 
CFString refactor / cleanup

This pull request:
- removes two unnecessary casts (from `CFIndex` to `usize` and back again)
- extracts the to rust string conversion: instead of doing it on-the-fly in the `Display` implementation (and thus only supporting the indirect `ToString` way to get a rust string from a `CFString`, which _always_ allocates) this implements `From<&CFString> for Cow<str>` allowing everyone the possibility of avoiding an allocation if the `CFString` is already formatted in UTF-8.

Rebase of #76 (note to self: next time, re-open first, then push).

cc @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-foundation-rs/220)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@TimNN @metajack @nox @jdm
You can’t perform that action at this time.