fix Datetime comparisons and add-seconds off-by-one#4
fix Datetime comparisons and add-seconds off-by-one#4carpentry-agent[bot] wants to merge 1 commit into
Conversation
- Add missing month check to Datetime.= - Replace broken or-based > and < with lexicographic cond chains - Remove spurious dec calls in add-seconds negative path, use floor division via new div- helper - Update tests and add coverage for comparison operators and negative second subtraction edge cases
There was a problem hiding this comment.
Build & Tests
Build: carp --check test/time.carp passes (type checking + macro expansion). Full compilation (carp -x) fails due to pre-existing tm_zone const mismatch on ARM Linux — this is a Carp compiler issue with the struct tm binding, not related to this PR.
CI: test (macos-latest) passes. No ubuntu check is configured (would hit the same tm_zone compilation error).
Findings
I traced through the arithmetic for all three fixes:
-
Datetime.=missing month (time.carp:868): Straightforward — added(= (month a) (month b)). Obviously correct. -
Datetime.>/Datetime.<broken logic (time.carp:879-920): The old(or ...)logic was fundamentally wrong —(> 2023-12-31 2024-01-01)returnedtruebecause(> 31 1)for the day field. The newcond-based lexicographic chains are correct: each field is checked in priority order (year → month → day → hours → minutes → seconds → nanoseconds), and comparison only proceeds to the next field when the current fields are equal. The implementations of>and<are properly symmetric. -
add-secondsoff-by-one (time.carp:250-275): The root cause analysis is sound. The old code had spurious(dec ...)calls that introduced off-by-one errors, compounded by C-style truncation division (/in Carp truncates toward zero, not toward negative infinity). The newdiv-helper (time.carp:245) correctly usesfloor(a/n)via floating-point conversion, matching the existingmod-pattern (time.carp:241-242). I verified the arithmetic:add-seconds -1from00:00:00: yields23:59:59(correct, old code gave wrong seconds)add-seconds -300from00:00:00: yields23:55:00(correct, old code gave23:54:59)add-seconds -3661from00:00:00: yields22:58:59(correct)
-
Tests (test/time.carp:568-621): The 8 new tests cover the three bug classes well — equality with different months, comparison across year boundaries, equal datetimes returning false for
>and<, and the two negative-seconds edge cases.
One minor note: div- could lose precision for very large integer arguments (> 2^53) due to the Double.from-int conversion, but since the function only ever receives seconds/minutes/hours/days values, this is not a practical concern.
Verdict: merge
All three bugs are real, correctly diagnosed, and properly fixed. The test coverage is good. The only compilation blocker is a pre-existing Carp compiler issue unrelated to this PR.
Summary
2024-01-15and2024-02-15compared as equal. Added(= (month a) (month b))to the comparison chain.(or ...)which returned true if any field was greater/less, producing wrong results (e.g.(> 2023-12-31 2024-01-01)was true because day 31 > 1). Also both were missing month comparison entirely. Replaced with proper lexicographic(cond ...)chains that compare year → month → day → hours → minutes → seconds → nanoseconds.(dec ...)calls on seconds, minutes, hours, and day calculations. The root cause was integer division truncating toward zero instead of flooring. Removed alldeccalls and introduced adiv-helper (matching the existingmod-) that uses floor division.Test plan
"23:55:00"instead of"23:54:59">with same year different month,<with year priority, equal datetimes false for both>and<carp --checkpassescarp-fmt -cpassesanglerpasses🤖 Generated with Claude Code