Add rawFractionalSeconds property to GeneralizedTime
#53
Conversation
|
@swift-server-bot add to allowlist |
Lukasa
added
the
🔼 needs-minor-version-bump
| } | ||
| } | ||
|
|
||
| /// The `ArraySlice` of bytes from which `fractionalSeconds` will be computed. (Preserved due to a possible overflow |
There was a problem hiding this comment.
Let's remove the parenthetical here, it's not really necessary.
| /// - minutes: The numerical minutes | ||
| /// - seconds: The numerical seconds | ||
| /// - rawFractionalSeconds: The `ArraySlice` of bytes from which `fractionalSeconds` will be computed. | ||
| /// (Preserved due to a possible overflow when computing a `Double` from this `ArraySlice`.) |
There was a problem hiding this comment.
Again, let's remove the parenthetical.
clintonpi
force-pushed
the
raw-fractional-seconds
branch
from
f765f31
to
7769923
Compare
| // Fractional seconds may not be negative and may not be 1 or more. | ||
| guard fractionalSeconds >= 0 && fractionalSeconds < 1 else { | ||
| throw ASN1Error.invalidASN1Object( | ||
| reason: "Invalid fractional seconds" |
There was a problem hiding this comment.
can we include fractionalSeconds in the reason?
| if lhs.fractionalSeconds < rhs.fractionalSeconds { return true } else if lhs.fractionalSeconds > rhs.fractionalSeconds { return false } | ||
|
|
||
| for (lhsByte, rhsByte) in zip(lhs.rawFractionalSeconds, rhs.rawFractionalSeconds) { | ||
| if lhsByte != rhsByte { | ||
| return lhsByte < rhsByte | ||
| } | ||
| } | ||
|
|
||
| // Since the above `zip` iteration stops at the length of the shorter `Sequence`, finally, | ||
| // compare the length of the two `Sequence`s. | ||
| return lhs.rawFractionalSeconds.count < rhs.rawFractionalSeconds.count; |
There was a problem hiding this comment.
Related to the discussion above about the source of truth. I think rawFractionalSeconds should be the source of truth and fractionalSeconds is just a cached/derived value from rawFractionalSeconds as it will always be more precise.
Therefore we might want to get rid of the lhs.fractionalSeconds < rhs.fractionalSeconds check. I'm not certain though as it might be a performance improvement. Is it guaranteed that there isn't a conflict between these two values? I guess so but I'm not sure. Either way, worth a comment that this is just an additional check to improve performance.
With that being said, how expensive is a double comparison compared to iterating over an array if the array is smallish?
There was a problem hiding this comment.
A double comparison is very cheap compared to the array iteration. As close to free as makes no odds for us here.
clintonpi
force-pushed
the
raw-fractional-seconds
branch
from
7769923
to
3cd4b39
Compare
Motivation: There is a possible overflow when computing a `Double` value for `fractionalSeconds` from an `ArraySlice` of bytes. Hence, access to this original `ArraySlice` is necessary. Modifications: - Add `rawFractionalSeconds` property to `GeneralizedTime`. - Add a new `internal init` to `GeneralizedTime` that accepts `rawFractionalSeconds` instead of `fractionalSeconds` and generates the later from the former. - Adjust the algorithm for computing `fractionalSeconds`, from a mathematical to a `String` computation. This is due to the precision mismatch between the mathematical computation and the native `Double` computation. This issue, which previously did not surface, now does, due to the newly included round-trip conversion from `fractionalSeconds` to `rawFractionalSeconds`. Result: - The `rawFractionalSeconds` property now provides the original `ArraySlice` from which `fractionalSeconds` was computed. - `GeneralizedTime` can now compute `fractionalSeconds` from the provided `rawFractionalSeconds`.
- Improve error and documentation. - Change the new `GeneralizedTime` `init` from `internal` to `public`.
clintonpi
force-pushed
the
raw-fractional-seconds
branch
from
f2b6bbe
to
4ffe69f
Compare
Motivation:
There is a possible overflow when computing a
Doublevalue forfractionalSecondsfrom anArraySliceof bytes. Hence, access to this originalArraySliceis necessary.Modifications:
rawFractionalSecondsproperty toGeneralizedTime.public inittoGeneralizedTimethat acceptsrawFractionalSecondsinstead offractionalSecondsand generates the latter from the former.fractionalSeconds, from a mathematical to aStringcomputation. This is due to the precision mismatch between the mathematical computation and the nativeDoublecomputation. This issue, which previously did not surface, now does, due to the newly included round-trip conversion fromfractionalSecondstorawFractionalSeconds._fractionalSecondsis a cached value and_rawFractionalSecondsis the source of truth for the numerical fractional seconds. (This holds directly for the newGeneralizedTimeinitthat takesrawFractionalSeconds. And it holds indirectly for the existinginitthat takesfractionalSeconds, as no information is lost in the conversion from_fractionalSecondsto_rawFractionalSeconds.)Result:
rawFractionalSecondsproperty now provides the originalArraySlicefrom whichfractionalSecondswas computed.GeneralizedTimecan now computefractionalSecondsfrom the providedrawFractionalSeconds.