refactor: base operators on in-place ones #919
Conversation
Operators should be implemented based on their in-place variants, not the other way around. In that way in-place operations won't allocate any new objects. See SwifterSwift#916 for more details. This PR allows us to reenable the swiftlint rule shorthand_operator. It also refactors other operators to use their in-place variant to reduce code duplication.
|
I changed all the other operators (except for the ones in |
Codecov Report
@@ Coverage Diff @@
## master #919 +/- ##
==========================================
- Coverage 92.84% 92.81% -0.04%
==========================================
Files 100 100
Lines 3634 3674 +40
==========================================
+ Hits 3374 3410 +36
- Misses 260 264 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Generated by 🚫 Danger |
| @@ -81,7 +83,9 @@ public extension CGPoint { | |||
| /// - rhs: CGPoint to subtract. | |||
| /// - Returns: result of subtract of the two given CGPoints. | |||
| static func - (lhs: CGPoint, rhs: CGPoint) -> CGPoint { | |||
| return CGPoint(x: lhs.x - rhs.x, y: lhs.y - rhs.y) | |||
There was a problem hiding this comment.
TBH I think this is better the way it was ... to me it is simpler :)
| @@ -51,7 +51,9 @@ public extension CGPoint { | |||
| /// - rhs: CGPoint to add. | |||
| /// - Returns: result of addition of the two given CGPoints. | |||
| static func + (lhs: CGPoint, rhs: CGPoint) -> CGPoint { | |||
| return CGPoint(x: lhs.x + rhs.x, y: lhs.y + rhs.y) | |||
| @@ -65,8 +67,8 @@ public extension CGPoint { | |||
| /// - lhs: self | |||
| /// - rhs: CGPoint to add. | |||
| static func += (lhs: inout CGPoint, rhs: CGPoint) { | |||
| // swiftlint:disable:next shorthand_operator | |||
| lhs = lhs + rhs | |||
| @@ -110,7 +114,9 @@ public extension CGPoint { | |||
| /// - scalar: scalar value. | |||
| /// - Returns: result of multiplication of the given CGPoint with the scalar. | |||
| static func * (point: CGPoint, scalar: CGFloat) -> CGPoint { | |||
| return CGPoint(x: point.x * scalar, y: point.y * scalar) | |||
| var result = point | |||
|
Also, don't forget the changelog entry :) |
There was a problem hiding this comment.
Unfortunately code duplication is inevitable in this project. One of the conditions is that any function should be copyable as-is, without having to copy any other functions*. This means that you can't use the in-place variants.
In some places it's also a lot more inefficient. E.g. in CGPoint.+(lhs:rhs:) instead of creating the new object directly with the added values, you're creating a copy of an object, jumping to another function in order to add the values via a reference and then jump back to return the updated object. It's longer, less efficient code that violates a primary directive of the project.
On the other hand, the updates to the in-place functions, e.g. CGPoint.+=(lhs:rhs:), are much better! I agree that they're more efficient and they also fix the copying issue mentioned.
* In the past this wasn't enforced so it exists in some places, but we don't want it to happen any more**.
** Even though I'm personally not in favour of it.
IMO, non-complex arithmetic operators such
+1 |
I'm more than happy to make an exception here. My other points still stand though :) |
|
Closing as I don't have the time to address the requirements. Feel free to reuse portions of this PR. |
Hey @sk- , no worries! |
Operators should be implemented based on their in-place variants, not the other way around. In that way in-place operations won't allocate any new objects.
See #916 for more details.
This PR allows us to reenable the swiftlint rule shorthand_operator. It also refactors other operators to use their in-place variant to reduce code duplication.
Checklist
@availableif not.