Surge
Surge copied to clipboard
Deprecation/Removal of `sqrt` variant with custom output storage?
tl;dr
A pair of functions seems to have been marked as public
at some point in the past, that should have remained internal
and have since been obsoleted. We should remove them from the public API.
What
Surge contains a somewhat odd function for calculating sqrt
by providing a custom output buffer:
/// Elemen-wise square root with custom output storage.
///
/// - Warning: does not support memory stride (assumes stride is 1).
public func sqrt<MI: UnsafeMemoryAccessible, MO: UnsafeMutableMemoryAccessible>(_ lhs: MI, into results: inout MO) where MI.Element == Float, MO.Element == Float {
return lhs.withUnsafeMemory { lhsMemory in
results.withUnsafeMutableMemory { rm in
precondition(lhsMemory.stride == 1 && rm.stride == 1, "sqrt doesn't support step values other than 1")
precondition(rm.count >= lhsMemory.count, "`results` doesnt have enough capacity to store the results")
vvsqrtf(rm.pointer, lhsMemory.pointer, [numericCast(lhsMemory.count)])
}
}
}
(Source)
Why
This pattern is nowhere else to be found in the framework and it looks to me like it was intended primarily for internal use as a base-implementation of the non-custom variant and possibly marked as public
by accident(?):
/// Elemen-wise square root.
///
/// - Warning: does not support memory stride (assumes stride is 1).
public func sqrt<C: UnsafeMemoryAccessible>(_ lhs: C) -> [Double] where C.Element == Double {
var results = [Double](repeating: 0.0, count: numericCast(lhs.count))
sqrt(lhs, into: &results)
return results
}
(Source)
(The method in question was added in this commit.)
How
As such I'd like to nominate both variants (Float
& Double
) of this function for deprecation with the next minor release, with removal on next major release.
What do you guys think, @mattt, @alejandro-isaza?
Update: with #119 merged we now have a proper sqrtInPlace
function for efficient non-allocating computation of sqrt
:
func sqrtInPlace<C: UnsafeMutableMemoryAccessible>(_ lhs: inout C) where C.Element == Float {
var elementCount: Int32 = numericCast(lhs.count)
lhs.withUnsafeMutableMemory { lm in
precondition(lm.stride == 1, "\(#function) doesn't support step values other than 1")
vvsqrtf(lm.pointer, lm.pointer, &elementCount)
}
}
(Source)
That sounds great, @regexident. I can't speak to the original implementation, but I agree with your proposal to deprecate and remove those functions.