Skip to content

New function to implement Data -> String fastpath#1

Open
alexwishart wants to merge 7 commits into
masterfrom
alex
Open

New function to implement Data -> String fastpath#1
alexwishart wants to merge 7 commits into
masterfrom
alex

Conversation

@alexwishart
Copy link
Copy Markdown
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
Comment thread DataToString.swift
}

func dataToString(data: Data) -> String {
let holder = Holder(data: data)
Copy link
Copy Markdown
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.

Comment thread DataToString.swift
func code(block: Int, loops: Int) -> () -> Void {
return {
var string: String?
var nsstring: NSString?
Copy link
Copy Markdown
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
Copy Markdown
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