Boris-Em / BEMSimpleLineGraph

Elegant Line Graphs for iOS. (Charting library)
MIT License
2.66k stars 382 forks source link

DataSource and Delegate use of `NSInteger` instead of `NSUInteger` in regards to Swift and Casting #296

Open Baza207 opened 7 years ago

Baza207 commented 7 years ago

I've just updated an old project to Swift 3.0 and had to swap to the "feature" branch to get the benefits of #214 to allow my project to build. To expand upon that fix though, I'd love to see a minor enhancement.

When it comes to Swift, uses of NSUInteger instead of just NSInteger for delegate calls such as:

- (NSUInteger)numberOfPointsInLineGraph:(BEMSimpleLineGraphView *)graph;

and

- (CGFloat)lineGraph:(BEMSimpleLineGraphView *)graph valueForPointAtIndex:(NSUInteger)index;

can cause issues in regards to casting.

These translate to UInt (as you'd expect), but because Swift is such a strict language, using them in a simple way, requires casting between UInt and Int (and vice versa).

For example:

func numberOfPoints(inLineGraph graph: BEMSimpleLineGraphView) -> UInt {
    return myArray.count
}

The above will show a build error unless you specifically cast myArray.count in Int(myArray.count).

func numberOfPoints(inLineGraph graph: BEMSimpleLineGraphView) -> UInt {
    return Int(myArray.count)
}

Likewise:

func lineGraph(_ graph: BEMSimpleLineGraphView, valueForPointAt index: UInt) -> CGFloat {

    let myItem = myArray[index]
    // Code using the item
}

This also fails unless you add an extra line of let index = Int(index) to make sure you're passing a Int to the array subscript.

func lineGraph(_ graph: BEMSimpleLineGraphView, valueForPointAt index: UInt) -> CGFloat {

    let index = Int(index)
    let myItem = myArray[index]
    // Code using the item
}

I understand why NSUInteger has been used, as an array is unlikely to have an index < 0, but this sadly does cause extra code issues for Swift users (minimal, I agree, but still). Also, this is going away from the standards that delegate calls in UITableViewDataSource and UICollectionViewDataSource use, where just NSInteger is used.

I'd love to hear what you think, and allow this to be considered, as it would help a lot with some unnecessary code for Swift users, with minimal impact for Objective-C users.

mackworth commented 7 years ago

Wow, that's pretty annoying. In ObjC, myArray.count is a NSUInteger as is index in myArray[index]. In fact, I changed these to NSUInteger (during an inspection for signed/unsigned issues) in other to fix those exact compiler errors in ObjC. I'll leave it up to the developers here, but I guess Swift is the future...

Baza207 commented 7 years ago

Yeah, it's a bit of a pain, but it's all to do with Swift being so "safe".

From my point of view it's a minor change to Obj-C that is a bigger change for Swift users. I know it means that it could cause a out of bounds error in Obj-C, but from my experience I've never seen a < 0 out of bound error in Obj-C. They're normally when the index is too large, rather than too small. 🙂

mackworth commented 7 years ago

Agreed, and it makes sense to put it back.

Let me just pedantically add that as Objc indices are unsigned (coerced if necessary, usually without warning), you'll never see a too-small index. In fact, if you have type checking turned on, you can safely protect before indexing with a single check with an NSUInteger, which takes two with an int.