diff --git a/Semantics.Music/Duration.cs b/Semantics.Music/Duration.cs
index 05f65c8b..722929d3 100644
--- a/Semantics.Music/Duration.cs
+++ b/Semantics.Music/Duration.cs
@@ -39,6 +39,7 @@ public sealed record Duration
/// The denominator; must be non-zero.
/// A new, reduced duration with a positive denominator.
/// Thrown when is 0.
+ /// Thrown when the reduced fraction does not fit in , as for int.MinValue/-1.
public static Duration Create(int numerator, int denominator)
{
if (denominator == 0)
@@ -46,32 +47,37 @@ public static Duration Create(int numerator, int denominator)
throw new ArgumentOutOfRangeException(nameof(denominator), "Denominator must be non-zero.");
}
- int sign = denominator < 0 ? -1 : 1;
- int n = numerator * sign;
- int d = denominator * sign;
- int g = Gcd(Math.Abs(n), d);
- g = g == 0 ? 1 : g;
- return new() { Numerator = n / g, Denominator = d / g };
+ return Reduce(numerator, denominator);
}
/// Returns the sum of this duration and another.
/// The duration to add.
/// The summed duration.
/// Thrown when is null.
+ /// Thrown when the reduced sum does not fit in .
public Duration Add(Duration other)
{
Ensure.NotNull(other);
- return Create((Numerator * other.Denominator) + (other.Numerator * Denominator), Denominator * other.Denominator);
+
+ // Over the least common denominator, so a run of sums grows only as fast as the lcm of the
+ // denominators seen, not their product. Every intermediate fits in long because each factor
+ // is an int.
+ long g = Gcd(Denominator, other.Denominator);
+ long numerator = (Numerator * (other.Denominator / g)) + (other.Numerator * (Denominator / g));
+ long denominator = Denominator / g * other.Denominator;
+ return Reduce(numerator, denominator);
}
/// Returns this duration multiplied by an integer factor.
/// The integer factor.
/// The scaled duration.
- public Duration Multiply(int factor) => Create(Numerator * factor, Denominator);
+ /// Thrown when the reduced product does not fit in .
+ public Duration Multiply(int factor) => Reduce((long)Numerator * factor, Denominator);
/// Returns this duration with a single augmentation dot (×3/2).
/// The dotted duration.
- public Duration Dotted() => Create(Numerator * 3, Denominator * 2);
+ /// Thrown when the reduced result does not fit in .
+ public Duration Dotted() => Reduce((long)Numerator * 3, (long)Denominator * 2);
/// Parses a fraction "n/d".
/// The fraction text.
@@ -120,7 +126,21 @@ public static bool TryParse(string? text, [System.Diagnostics.CodeAnalysis.NotNu
public override string ToString() =>
$"{Numerator.ToString(CultureInfo.InvariantCulture)}/{Denominator.ToString(CultureInfo.InvariantCulture)}";
- private static int Gcd(int a, int b)
+ ///
+ /// Reduces a fraction computed in and narrows it to , throwing
+ /// rather than wrapping when it does not fit.
+ ///
+ private static Duration Reduce(long numerator, long denominator)
+ {
+ long sign = denominator < 0 ? -1 : 1;
+ long n = numerator * sign;
+ long d = denominator * sign;
+ long g = Gcd(Math.Abs(n), d);
+ g = g == 0 ? 1 : g;
+ return new() { Numerator = checked((int)(n / g)), Denominator = checked((int)(d / g)) };
+ }
+
+ private static long Gcd(long a, long b)
{
while (b != 0)
{
diff --git a/Semantics.Test/Music/DurationTests.cs b/Semantics.Test/Music/DurationTests.cs
index cefa849f..ae375fb0 100644
--- a/Semantics.Test/Music/DurationTests.cs
+++ b/Semantics.Test/Music/DurationTests.cs
@@ -42,6 +42,66 @@ public void Add_SumsFractions()
Assert.AreEqual(Duration.Create(3, 4), Duration.Half.Add(Duration.Quarter));
}
+ [TestMethod]
+ public void Add_TenTupletDurations_ThrowsOverflowInsteadOfWrapping()
+ {
+ // 1/3 + 1/5 + ... + 1/31 is exactly n/100280245065, which no int fraction can hold. It used
+ // to wrap to -505842482/1495997257 and report a negative total.
+ int[] denominators = [3, 5, 7, 11, 13, 17, 19, 23, 29, 31];
+ Duration total = Duration.Create(0, 1);
+ _ = Assert.ThrowsExactly(() =>
+ {
+ foreach (int denominator in denominators)
+ {
+ total = total.Add(Duration.Create(1, denominator));
+ }
+ });
+ Assert.IsTrue(total.Numerator > 0, $"A partial sum wrapped to {total}");
+ }
+
+ [TestMethod]
+ public void Add_WhoseCommonDenominatorFits_SumsExactly()
+ {
+ // The product of the denominators passes int.MaxValue but their lcm does not, so the sum is
+ // representable. Multiplying the denominators first wrapped these.
+ Assert.AreEqual(Duration.Create(1, 32768), Duration.Create(1, 65536).Add(Duration.Create(1, 65536)));
+
+ Duration eightTuplets = Duration.Create(0, 1);
+ foreach (int denominator in new[] { 3, 5, 7, 11, 13, 17, 19, 23 })
+ {
+ eightTuplets = eightTuplets.Add(Duration.Create(1, denominator));
+ }
+
+ Duration total = eightTuplets.Add(Duration.Create(1, 3 * 5 * 7 * 11 * 13 * 17 * 19));
+ Assert.AreEqual(0, 111546435 % total.Denominator, $"{total} should be over a divisor of the lcm");
+ Assert.AreEqual(eightTuplets.AsWholeNotes + (1.0 / 4849845), total.AsWholeNotes, 1e-12);
+ }
+
+ [TestMethod]
+ public void Add_SharedDenominators_StayReduced()
+ {
+ // Mixing triplets and plain values reuses denominators, so the sum stays over their lcm.
+ Duration total = Duration.Create(1, 12).Add(Duration.Create(1, 6)).Add(Duration.Create(1, 4)).Add(Duration.Create(1, 12));
+ Assert.AreEqual(Duration.Create(7, 12), total);
+ }
+
+ [TestMethod]
+ public void Arithmetic_ThatDoesNotFitInInt_ThrowsOverflow()
+ {
+ _ = Assert.ThrowsExactly(() => Duration.Create(2, 1).Multiply(int.MaxValue));
+ _ = Assert.ThrowsExactly(() => Duration.Create(int.MaxValue, 1).Dotted());
+ _ = Assert.ThrowsExactly(() => Duration.Create(1, int.MaxValue).Add(Duration.Create(1, int.MaxValue - 1)));
+ _ = Assert.ThrowsExactly(() => Duration.Create(int.MinValue, -1));
+ }
+
+ [TestMethod]
+ public void Arithmetic_WhoseReducedResultFits_DoesNotThrow()
+ {
+ // The unreduced product passes int.MaxValue, but the reduced result does not.
+ Assert.AreEqual(Duration.Create(1, 1), Duration.Create(1, int.MaxValue).Multiply(int.MaxValue));
+ Assert.AreEqual(Duration.Create(int.MinValue, 1), Duration.Create(int.MinValue, 1));
+ }
+
[TestMethod]
public void Create_RejectsZeroDenominator()
{