Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yams: add an alias to explicitly dispatch pow #281

Merged
merged 1 commit into from
Oct 26, 2020
Merged

Yams: add an alias to explicitly dispatch pow #281

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions Sources/Yams/Representer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,17 @@ import Foundation
import ucrt
#endif

#if os(iOS) || os(macOS) || os(watchOS) || os(tvOS)
import Darwin
fileprivate let cpow: (_: Double, _: Double) -> Double = Darwin.pow
#elseif os(Windows)
import ucrt
fileprivate let cpow: (_: Double, _: Double) -> Double = ucrt.pow
#else
import Glibc
fileprivate let cpow: (_: Double, _: Double) -> Double = Glibc.pow
#endif
Comment on lines +17 to +26
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be simplified to this?

#if canImport(Darwin)
private let cpow: (_: Double, _: Double) -> Double = Darwin.pow
#elseif canImport(ucrt)
private let cpow: (_: Double, _: Double) -> Double = ucrt.pow
#elseif canImport(Glibc)
private let cpow: (_: Double, _: Double) -> Double = Glibc.pow
#else
#error("Unsupported platform")
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is really a simplification - it's different semantics. If I were to have a C module or a Swift module with the name Darwin on Windows, this would cause a build failure. I can certainly change it to that if you feel strongly though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly, and see the risk of name collisions, although I would advise against naming new modules Darwin for a slew of reasons. I consider it a simplification in the sense that if new platforms were introduced (e.g. os(HomePod)) we wouldn't have to touch this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I still think that introduction of an os(Darwin) or vendor(Apple) is better. However, that's not the current reality. I'll update this in a few minutes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to update, I'll merge as-is.


public extension Node {
/// Initialize a `Node` with a value of `NodeRepresentable`.
///
Expand Down Expand Up @@ -130,13 +141,11 @@ private extension TimeInterval {
func separateFractionalSecond(withPrecision precision: Int) -> (integral: TimeInterval, fractional: Int) {
var integral = 0.0
let fractional = modf(self, &integral)
// Can't use `pow` free function due to https://bugs.swift.org/browse/TF-1203.
// TODO: Remove condition after that bug is fixed.
#if canImport(TensorFlow)
let radix = Double.pow(10.0, Double(precision))
#else
let radix = pow(10.0, Double(precision))
#endif

// TODO(TF-1203): Can't use `pow` free function due to
// https://bugs.swift.org/browse/TF-1203.
let radix = cpow(10.0, Double(precision))

let rounded = Int((fractional * radix).rounded())
let quotient = rounded / Int(radix)
return quotient != 0 ? // carry-up?
Expand Down