Skip to content

Conversation

jasonbobier
Copy link

This PR does the following:

Simplifies GCD to gcd(T, T) -> T.Magnitude, which has the following benefits.

  • Removes the potential trap when T.Magnitude is not representable in T. This allows the user of the api to decide how to handle this case instead of discarding the valid result and causing the program to crash
  • Removes a couple of lines of other edge case code that are no longer needed if we return T.Magnitude.

Updates the test cases to use SwiftTesting. It also removes one test case that was 2's complement specific.

Renames the source code files to more descriptive names similar to the rest of the repo.

Changed GCD.swift to GreatestCommonDivisor.swift.
Chagned GCDTests.swift to GreatestCommonDivisorTests.swift
Updated gcd to return Magnitude.
Updated tests to SwiftTesting.
@stephentyrone
Copy link
Member

I'm still debating T vs T.Magnitude; my instinct is to agree with @xwu that the homogeneous form is more approachable and if you want magnitude you can do gcd(a.magnitude, b.magnitude), but I'm going to ponder it a little longer.

@jasonbobier
Copy link
Author

I'm still debating T vs T.Magnitude; my instinct is to agree with @xwu that the homogeneous form is more approachable and if you want magnitude you can do gcd(a.magnitude, b.magnitude), but I'm going to ponder it a little longer.

Totally get that. I do worry that it will add program crashes that can be avoided if people (especially those new to the language) don't think about using (a|b).magnitude whereas we can force them to think about it if we return T.Magnitude and avoid the crashes. We could add something to the api description, but then my thoughts lead to "why don't we just prevent the crash in the first place with the api rather than just documenting it..."

@jasonbobier
Copy link
Author

jasonbobier commented Aug 17, 2025

@inlinable
public func gcd<T: BinaryInteger>(_ a: T, _ b: T) -> T {
    if a.magnitude == 1 || b.magnitude == 1 { return 1 }

    let gcd = greatestCommonDivisor(a.magnitude, b.magnitude)
    
    // Try to convert result to T.
    if let result = T(exactly: gcd) { return result }
    // If that fails, produce a diagnostic.
    fatalError("GCD (\(gcd)) is not representable as \(T.self).")
}


/// The [greatest common divisor][gcd] of `a` and `b`.
///
/// If both inputs are zero, the result is zero. If one input is zero, the
/// result is the absolute value of the other input.
///
/// [gcd]: https://en.wikipedia.org/wiki/Greatest_common_divisor
@inlinable
public func greatestCommonDivisor<T: BinaryInteger>(_ a: T, _ b: T) -> T.Magnitude {
    var x = a
    var y = b

    if x.magnitude < y.magnitude {
        swap(&x, &y)
    }

    // Euclidean algorithm for GCD. It's worth using Lehmer instead for larger
    // integer types, but for now this is good and dead-simple and faster than
    // the other obvious choice, the binary algorithm.
    while y != 0 {
        (x, y) = (y, x % y)
    }

    return x.magnitude
}

btw, my first pass was purely additive which may be the solution that we want. (Note that this isn't that version, just a quick code writeup to show the idea.)

@jasonbobier
Copy link
Author

Incorporating @xwu's thoughts, I think that we should rework this into gcd, greatestCommonDivisorReportingOverflow, and greatestCommonDivisorFullWidth and that will make it consistent with the lcm code and cover all of the uses that come to mind.

@jasonbobier
Copy link
Author

Here's the update with gcd, greatestCommonDivisorReportingOverflow, and greatestCommonDivisorFullWidth. If everyone likes this, I'll merge it into the jasonbobier/gcd branch.

jasonbobier#2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants