Skip to content

fix Datetime comparisons and add-seconds off-by-one#4

Open
carpentry-agent[bot] wants to merge 1 commit into
masterfrom
claude/fix-datetime-comparisons
Open

fix Datetime comparisons and add-seconds off-by-one#4
carpentry-agent[bot] wants to merge 1 commit into
masterfrom
claude/fix-datetime-comparisons

Conversation

@carpentry-agent
Copy link
Copy Markdown

Summary

  • Datetime.= missing month: The equality operator skipped the month field, so dates like 2024-01-15 and 2024-02-15 compared as equal. Added (= (month a) (month b)) to the comparison chain.
  • Datetime.> and Datetime.< broken logic: Both used (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.
  • add-seconds off-by-one with negative values: The negative path had spurious (dec ...) calls on seconds, minutes, hours, and day calculations. The root cause was integer division truncating toward zero instead of flooring. Removed all dec calls and introduced a div- helper (matching the existing mod-) that uses floor division.

Test plan

  • Fixed existing test: expected "23:55:00" instead of "23:54:59"
  • Added equality tests: same dates equal, different months not equal
  • Added comparison tests: > with same year different month, < with year priority, equal datetimes false for both > and <
  • Added add-seconds edge cases: subtract 1 second from midnight, subtract 3661 seconds (1h1m1s)
  • carp --check passes
  • carp-fmt -c passes
  • angler passes

🤖 Generated with Claude Code

- 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
Copy link
Copy Markdown

@carpentry-reviewer carpentry-reviewer Bot left a comment

Choose a reason for hiding this comment

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

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:

  1. Datetime.= missing month (time.carp:868): Straightforward — added (= (month a) (month b)). Obviously correct.

  2. Datetime.> / Datetime.< broken logic (time.carp:879-920): The old (or ...) logic was fundamentally wrong — (> 2023-12-31 2024-01-01) returned true because (> 31 1) for the day field. The new cond-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.

  3. add-seconds off-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 new div- helper (time.carp:245) correctly uses floor(a/n) via floating-point conversion, matching the existing mod- pattern (time.carp:241-242). I verified the arithmetic:

    • add-seconds -1 from 00:00:00: yields 23:59:59 (correct, old code gave wrong seconds)
    • add-seconds -300 from 00:00:00: yields 23:55:00 (correct, old code gave 23:54:59)
    • add-seconds -3661 from 00:00:00: yields 22:58:59 (correct)
  4. 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.

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.

0 participants