Replaced unnecessary NSObjectProtocol

[jjatie]

class is better as it allows Swift classes to avoid the NSObject overhead.
the @objc tag on any classes that may adopt the protocol enforces NSObject adoption.

[codecov-io]

Codecov Report

Merging #2629 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2629   +/-   ##
=======================================
  Coverage   19.65%   19.65%           
=======================================
  Files         112      112           
  Lines       13717    13717           
=======================================
  Hits         2696     2696           
  Misses      11021    11021

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6698fd3...26965e1. Read the comment docs.

[liuxuan30]

Thank you. Can you provide a little more details? e.g. I don't follow swift trends sometime, if we use class, do we need to add @objc tag to use the classes in ObjC projects?
I remember Swift 4 does not suggest add @objc for the whole class.

[jjatie]

Swift 4 doesn't change the rules on how things should be exposed to objective-c. Swift 4 removes most implicit @objc in most places requiring it be made explicit.

The changes in this pull request do not modify any @objc tags, but removes the NSObjectProtocol requirements on certain protocols in Charts. There are a few reasons for this:

1. the methods of NSObjectProtocol are not being used in Charts for those protocols.
2. Any objective-c class must inherit from NSObject anyway.
3. the most important is that it allows Swift classes adopting the protocols to be more performant as they no long need to inherit the overhead of NSObject

[jjatie]

Also, this doesn't impact any existing implementations of the framework.

[jjatie]

The only reason I mentioned @objc tags in the pull request is because the protocols changed are adopted by classes in the Charts framework which have the @objc tag. But it really doesn't matter, so perhaps I shouldn't have mentioned the tags.

[liuxuan30]

Got it. @petester42 any comments to get this merged? Need you or @danielgindi is here reviewing such foundermental changes

[jjatie]

@petester42 any comments on this one?

[pmairoldi]

using : class seems to be somewhat different than NSObjectProtocol. :class says that the protocol can only be implemented by classes. NSObjectProtocol is just a normal protocol that anyone can adhere too. For example you can have a protocol that adheres to class and it could not be an NSObject. In this case we expect it to have the interface of an NSObject so we are explicitly saying that it needs to be one.

The following is valid:

protocol TestProtocol: class {

}

class Test: TestProtocol {

}

And this is not:

protocol TestProtocol: NSObjectProtocol {

}

class Test: TestProtocol {

}

The code above will throw an error saying that it does not conform to NSObjectProtocol.

This doesn't really matter since the protocol is marked @objc but if it wasn't these 2 would not be the same. It think it's best to not include this change since to @objc stuff then using : class may not work the way we intended it to.

[jjatie]

While it is technically possible for any type to adopt NSObjectProtocol, if it is not a class it is being used improperly. From the documentation for NSObjectProtocol:

An object that conforms to this protocol can be considered a first-class object. Such an object can be asked about its:

Class, and the place of its class in the inheritance hierarchy.

Conformance to protocols.

Ability to respond to a particular message.

The Cocoa root class NSObject adopts this protocol, so all objects inheriting from NSObject have the features described by this protocol

While it's technically possible to make alternative [to NSObject] Objective-C objects NSObjectProtocol one should not be doing so, in which case requiring protocols to only adhere to class is sufficient. The only times where this is not sufficient is when protocol extensions are relying on NSObjectProtocol methods or variables which none are in Charts so while in:

protocol ClassProtocol: class { }
class Test1: ClassProtocol { }

protocol ObjCClassProtocol: NSObjectProtocol { }
class Test2: ObjCClassProtocol { }
Test1 != Test2

It doesn't matter because in all cases within Charts, we don't need any methods/variables of NSObjectProtocol so we can omit it.

There is one thing I left out which is that we don't really need NSObjectProtocol or class because by tagging them with @objc that allows all Swift types to adopt them and exposes them to Obj-C for NSObject subclasses to adopt them. The only reason I chose to enforce class was in case the framework is relying on reference semantics.

[jjatie]

What is the status of this PR?

[pmairoldi]

After rereading your comments. Seems fine to me.

[pmairoldi]

Thanks @jjatie