JohnEstropia / GCDKit

Grand Central Dispatch simplified with swift.
MIT License
320 stars 39 forks source link

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

Closed ra1028 closed 8 years ago

ra1028 commented 8 years ago

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 :bow:

JohnEstropia commented 8 years ago

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

public static func createSerial(label: String? = nil) -> GCDQueue
ra1028 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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.

JohnEstropia commented 8 years ago

@ra1028 Thanks!