Any critiques about these extensions on `Duration`?

Using Swift
1

I'm wondering if anyone has any concrete critiques about these extensions on Duration. I was pleasantly surprised to discover that Duration automatically normalizes input, which led me to believe that the two methods I've defined below are logically sound. Still, I felt a bit uneasy about them and so I'm turning to the forum to ask for any and all critiques before I start using them. Thanks!

///
@available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
extension Duration {
    
    ///
    public static prefix func - (operand: Self) -> Self {
        .init(
            secondsComponent: -operand.components.seconds,
            attosecondsComponent: -operand.components.attoseconds
        )
    }
}

///
@available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
extension Duration {
    
    ///
    public func scaled (by scaleFactor: Int) -> Self {
        .init(
            secondsComponent: self.components.seconds * Int64(scaleFactor),
            attosecondsComponent: self.components.attoseconds * Int64(scaleFactor)
        )
    }
}
2

I think the negation is fine but the scaled(by:) operation could overflow on the attoseconds multiplication even when the ultimate result would be representable, no?

3

If you're referring to that the scaled attoseconds are more likely to overflow because common durations (like 1ms) are represented using an already-high attosecond count then I think you're right, but to be clear it is not a problem to initialize a duration with more attoseconds than there are in a second. The following test passes:

///
@available(macOS 13.0, iOS 16.0, watchOS 9.0, tvOS 16.0, *)
func test_attosecondsOverflow () throws {
    
    ///
    let (seconds, attoseconds) =
        Duration(
            secondsComponent: 0,
            attosecondsComponent: .max
        )
            .components
    
    ///
    XCTAssertEqual(seconds, 9)
    XCTAssertEqual(attoseconds, 223372036854775807)
}
4

Yes I specifically mean that the attosecond multiplication could overflow the Int64 multiplication even though the scaling of the Duration value would be hypothetically valid. E.g. 1e18-1 attoseconds scaled by 1e18 should be 1e18-1 seconds but will instead overflow.

1 Like
5

Sorry yes, re-reading what you wrote in the first place I realize that your point was unambiguous and clearly correct. I think that an implementation which accounts for this would probably want to make use of the square-root of the largest representable square number. Nothing like Int.maxSquare exists already, right?

EDIT: Nevermind, I think my thinking on that is confused... I'll go back to the drawing board.

EDIT 2: I can't think of a correct implementation that doesn't resort to repetitive addition, so for now I'm going to accept the flaw in my implementation since at least it won't give any incorrect output (IIUC).

6

why don’t you just subtract from Duration.zero?

extension Duration
{
    @inlinable public static prefix 
    func - (self:Self) -> Self 
    {
        .zero - self
    }
}

?

i’ll also point out that Duration has a *(_:_:) operator already.

4 Likes
7

Duration already provides * overloads for multiplying with scale factors. (Slightly weirdly, the duration has to be the first operand and the factor the second, but oh well.)

extension Duration {
    func scaled(by scaleFactor: Int) -> Self { self * scaleFactor }
}

The implementation simply widens the factor to a 128 bit integer and then forwards to the full wide multiplication algorithm -- there is definitely room for improvement there.

4 Likes
8

As @lorentey noted, scaled(by:) is superfluous. We should add the other ordering for *, which I'm planning to roll into a catch-all proposal updating the Duration API.

9 Likes