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

New function to implement Data -> String fastpath #1

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

alexwishart
Copy link
Collaborator

Created new class which holds reference to a pointer This is so object can be passed to StringCore when it is created and acts as its owner and ensures memory is cleaned up appropriately. This behaviour might be possible using other core Swift classes, but hasn't been fully investigated.

@alexwishart alexwishart requested a review from djones6 August 24, 2017 14:14
@@ -129,10 +151,26 @@ func makeStringB(data: Data) -> String {
} ?? ""
}

func dataToString(data: Data) -> String {
let holder = Holder(data: data)
Copy link
Owner

Choose a reason for hiding this comment

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

Need to either add a condition here that only goes down this path if the Data's encoding is ASCII / UTF8 (similar to Philippe's code). The condition might belong in here, or rename this function to indicate that it is specifically an ASCII fastpath.

// Block to be scheduled
func code(block: Int, loops: Int) -> () -> Void {
return {
var string: String?
var nsstring: NSString?
Copy link
Owner

Choose a reason for hiding this comment

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

can you remove this (unused) ref + the commented block? I suspect method 0 is also not required.

Copy link
Owner

@djones6 djones6 left a comment

Choose a reason for hiding this comment

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

Couple of naming / cleanup comments to address.

Also, I think it would be useful to test this code converting Data from a variety of different encodings to check that they are correctly converted. It may be that plumbing this into Foundation and then running the tests would be sufficient - we'd need to see whether the existing tests cover conversion between different encodings.

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