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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the type of custom queue label from String to optional String #9

Merged
merged 2 commits into from Mar 4, 2016

Conversation

ra1028
Copy link
Contributor

@ra1028 ra1028 commented Mar 3, 2016

Hi, @JohnEstropia
Thanks for your great lib.

I changed the type of label of the function that creates custom queue to UnsafePointer, and applied the initial value to nil.

This changes is based on the following description in the 481-483 lines of dispatch/queue.h:

* @param label
* A string label to attach to the queue.
* This parameter is optional and may be NULL.

But it's OK to close this PR without merge, if you feel not good 馃檱

@JohnEstropia
Copy link
Owner

@ra1028 Thanks for the pull request!
Will it work if we use optional strings instead?

public static func createSerial(label: String? = nil) -> GCDQueue

@ra1028
Copy link
Contributor Author

ra1028 commented Mar 4, 2016

@JohnEstropia
Indeed, label just works for debugging purposes, so we should use optional string.
It will implements are following, because Optional type isn't convertible with UnsafePointer.

private static func createCustom(isConcurrent isConcurrent: Bool, label: String? = nil, targetQueue: GCDQueue?) -> GCDQueue {

        let attr = isConcurrent ? DISPATCH_QUEUE_CONCURRENT : DISPATCH_QUEUE_SERIAL
        let dispatchQueue = label.map {

            dispatch_queue_create($0, attr)
            } ?? dispatch_queue_create(nil, attr)
        let queue = GCDQueue.Custom(dispatchQueue)

        if let target = targetQueue {

            dispatch_set_target_queue(queue.dispatchQueue(), target.dispatchQueue())
        }
        return queue
}

Will that be OK?

@ra1028
Copy link
Contributor Author

ra1028 commented Mar 4, 2016

And, have you thought about adding the case to GCDQueue like this?

public enum GCDQueue {
   ...
   /**
   A user-created serial queue without label and target queue.
   */
   case Serial

   /**
   A user-created Concurrent queue without label and target queue.
   */
   case Concurrent
}

@JohnEstropia
Copy link
Owner

I think just casting the string to NSString should be shorter.

let queue = GCDQueue.Custom(
    dispatch_queue_create(
        (label as? NSString)?.UTF8String,
        (isConcurrent ? DISPATCH_QUEUE_CONCURRENT : DISPATCH_QUEUE_SERIAL)
    )
)

And, have you thought about adding the case to GCDQueue like this?

We can't make Serial and Concurrent enum values because we need to keep a reference to the created dispatch_queue_t.
The rest of the queues (.Main, .Default, .Background, etc) are all global queues, so we can get their dispatch_queue_ts with dispatch_get_main_queue() and dispatch_get_global_queue(...).

@ra1028
Copy link
Contributor Author

ra1028 commented Mar 4, 2016

@JohnEstropia
I see, thanks !
Just one thing:

(label as? NSString)?.UTF8String

A compile-error will also occur from this line.
So, I've change it to following:

(label as NSString?)?.UTF8String ?? nil

Please confirm that.

@ra1028 ra1028 changed the title Change the type of custom queue label from String to UnsafePointer<Int8> Change the type of custom queue label from String to optional String Mar 4, 2016
@JohnEstropia
Copy link
Owner

@ra1028 Thanks!

JohnEstropia added a commit that referenced this pull request Mar 4, 2016
Change the type of custom queue label from String to optional String
@JohnEstropia JohnEstropia merged commit 97046f6 into JohnEstropia:master Mar 4, 2016
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.

None yet

2 participants