-
Notifications
You must be signed in to change notification settings - Fork 28
[210_12] feat(srfi-19): 初始化 time 和 date 结构,实现部分函数 #396
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.
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
base: main
Are you sure you want to change the base?
Conversation
d2ba472 to
b0c90a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements SRFI-19 time and date support for Goldfish Scheme by adding C++17 chrono-based glue functions and comprehensive Scheme implementations of time and date structures with formatting capabilities.
Changes:
- Added C++ glue functions for time operations using std::chrono (get-time-of-day, monotonic-nanosecond, clock resolutions)
- Implemented SRFI-19 time and date record types with accessors and basic operations
- Implemented date-to-string formatting with extensive format specifier support
- Added comprehensive test suite covering time/date creation, accessors, and formatting
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/goldfish.hpp | Adds C++ glue functions using std::chrono for time operations and clock resolution queries |
| goldfish/scheme/time.scm | Refactors current-second and current-jiffy to use new glue functions, exports clock resolution constants |
| goldfish/srfi/srfi-19.scm | Implements SRFI-19 time/date types, current-time, time-resolution, date->string with format specifiers, and leap second handling |
| tests/goldfish/liii/time-test.scm | Comprehensive test suite for time/date constants, constructors, accessors, formatting, and error conditions |
| devel/210_12.md | Development documentation describing the implementation approach and testing instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (cond | ||
| ((null? table) 10) ; 理论值 | ||
| ((>= s (caar table)) (cdar table)) | ||
| (else (loop (cdr table)))))) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function uses loop but the binding is named lp. This should be (lp (cdr table)) to match the named let binding on line 218.
| (else (loop (cdr table)))))) | |
| (else (lp (cdr table)))))) |
| ((#\I) (let* ((hr (date-hour date)) | ||
| (hr* (if (> hr 12) (- hr 12) hr))) | ||
| (display (priv:padding hr* pad-with 2) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 12-hour conversion logic is incorrect. When the hour is exactly 12, it should remain 12 (noon/midnight display as 12, not 0). The condition should be: if hr is 0, use 12; otherwise if hr > 12, use hr - 12; otherwise use hr. Currently, hour 12 will be displayed as 12 which is correct, but hour 0 will be displayed as 0 instead of 12.
| (priv:date-printer date format-string | ||
| format-string-port port))))) | ||
|
|
||
| (define* (date->string date (format-string "~c")) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type check for the date parameter. The function should verify that date is actually a date object and raise a wrong-type-arg error if not, consistent with other SRFI-19 procedures.
| (define* (date->string date (format-string "~c")) | |
| (define* (date->string date (format-string "~c")) | |
| (unless (date? date) | |
| (wrong-type-arg 'date->string 1 "date" date)) |
| (priv:date-printer date format-string | ||
| format-string-port port))))) | ||
|
|
||
| (define* (date->string date (format-string "~c")) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type check for the format-string parameter. The function should verify that format-string is actually a string and raise a wrong-type-arg error if not, as tested in the test file at lines 788-789.
| (define* (date->string date (format-string "~c")) | |
| (define* (date->string date (format-string "~c")) | |
| (unless (string? format-string) | |
| (wrong-type-arg 'date->string format-string)) |
|
|
||
| ### What | ||
|
|
||
| 1. 新增 glue:`g_gettimeofday`, `g_monotonic-nanosecond`, `g_steady-clock-resolution`, `g_system-clock-resolution` |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the documentation: it says g_gettimeofday but the actual function name is g_get-time-of-day (with hyphens instead of no separators).
| 1. 新增 glue:`g_gettimeofday`, `g_monotonic-nanosecond`, `g_steady-clock-resolution`, `g_system-clock-resolution` | |
| 1. 新增 glue:`g_get-time-of-day`, `g_monotonic-nanosecond`, `g_steady-clock-resolution`, `g_system-clock-resolution` |
| s7_make_typed_function (sc, s_get_time_of_day, f_get_time_of_day, 0, 0, false, d_get_time_of_day, NULL)); | ||
|
|
||
| const char* s_monotonic_nanosecond= "g_monotonic-nanosecond"; | ||
| const char* d_monotonic_nanosecond= "(g_monotonic-nanosecond): () => integer, TODO"; |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation string should be updated to describe what this function actually returns. It currently says "TODO" but should explain that it returns the steady clock's monotonic nanoseconds since an unspecified epoch.
| const char* d_monotonic_nanosecond= "(g_monotonic-nanosecond): () => integer, TODO"; | |
| const char* d_monotonic_nanosecond= "(g_monotonic-nanosecond): () => integer, returns the steady clock's monotonic nanoseconds since an unspecified epoch"; |
| ;; 归前一年:判断前一年52周还是53周(周四年首=53周) | ||
| (if (> (priv:week-day 1 1 (- year 1)) 4) 52 53)) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ISO-8601 week number logic at line 399 has an incorrect condition. For ISO-8601, a year has 53 weeks if January 1st falls on a Thursday (4) OR if it's a leap year and January 1st falls on a Wednesday (3). The current check (> (priv:week-day 1 1 (- year 1)) 4) tests if the first day of the previous year is greater than 4 (Friday or Saturday), which would give 52 weeks. This appears backwards - it should check if the previous year's Jan 1 was on Thursday (or Wednesday for leap years) to determine 53 weeks.
| ;; 归前一年:判断前一年52周还是53周(周四年首=53周) | |
| (if (> (priv:week-day 1 1 (- year 1)) 4) 52 53)) | |
| ;; 归前一年:判断前一年52周还是53周 | |
| ;; ISO-8601: 前一年有53周当且仅当:该年1月1日是周四, | |
| ;; 或者该年是闰年且1月1日是周三。 | |
| (let* ((prev-year (- year 1)) | |
| (prev-jan1-wday (priv:week-day 1 1 prev-year)) | |
| (prev-leap? | |
| (or (and (zero? (modulo prev-year 4)) | |
| (not (zero? (modulo prev-year 100)))) | |
| (zero? (modulo prev-year 400))))) | |
| (if (or (= prev-jan1-wday 4) | |
| (and prev-leap? (= prev-jan1-wday 3))) | |
| 53 | |
| 52))) |
| (import (rename (scheme time) | ||
| (get-time-of-day glue:get-time-of-day) | ||
| (monotonic-nanosecond glue:monotonic-nanosecond)) | ||
| (only (srfi srfi-13) string-pad string-tokenize string-trim-right) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library uses receive (lines 225, 232, 241, 442) but doesn't import it. It should import receive from (srfi srfi-8) or another module that provides it.
| (only (srfi srfi-13) string-pad string-tokenize string-trim-right) | |
| (only (srfi srfi-13) string-pad string-tokenize string-trim-right) | |
| (only (srfi srfi-8) receive) |
| (let* ((str (number->string n)) | ||
| (str-len (string-length str))) | ||
| (cond | ||
| ((or (> str-len len) (not pad-with)) str) | ||
| (else (string-pad str len pad-with))))) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The padding function doesn't handle negative numbers correctly. When a negative number is converted to a string, the minus sign becomes part of the string, and padding will insert zeros/spaces after the minus sign (e.g., "-001" instead of "-01"). For proper formatting, the sign should be extracted first, and padding applied to the absolute value, then the sign prepended.
| (let* ((str (number->string n)) | |
| (str-len (string-length str))) | |
| (cond | |
| ((or (> str-len len) (not pad-with)) str) | |
| (else (string-pad str len pad-with))))) | |
| (let* ((str (number->string n)) | |
| (str-len (string-length str)) | |
| (negative? (and (> str-len 0) | |
| (char=? (string-ref str 0) #\-))) | |
| (sign (if negative? "-" "")) | |
| (core (if negative? | |
| (substring str 1 str-len) | |
| str)) | |
| (core-len (string-length core)) | |
| ;; For negative numbers, the requested length includes the sign, | |
| ;; so we reserve one character for it when padding the digits. | |
| (target-len (if negative? | |
| (max 0 (- len 1)) | |
| len))) | |
| (cond | |
| ((or (> (+ core-len (if negative? 1 0)) len) | |
| (not pad-with)) | |
| str) | |
| (else | |
| (string-append | |
| sign | |
| (string-pad core target-len pad-with)))))) |
| (import (rename (scheme time) | ||
| (get-time-of-day glue:get-time-of-day) | ||
| (monotonic-nanosecond glue:monotonic-nanosecond)) | ||
| (only (srfi srfi-13) string-pad string-tokenize string-trim-right) |
Copilot
AI
Feb 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library uses floor/ (lines 225, 443) but doesn't import it. It should import floor/ from (scheme base) or another module that provides it.
| (only (srfi srfi-13) string-pad string-tokenize string-trim-right) | |
| (only (srfi srfi-13) string-pad string-tokenize string-trim-right) | |
| (only (scheme base) floor/) |
[210_12]
(srfi srfi-19)任务相关的代码文件
src/goldfish.hpptests/goldfish/liii/time-test.scmgoldfish/scheme/time.scmgoldfish/srfi/srfi-19.scm如何测试
2026/1/30 初始化 time 和 date 结构
What
g_gettimeofday,g_monotonic-nanosecond,g_steady-clock-resolution,g_system-clock-resolutioncurrent-time,date->string等Why
扩充时间与日期的支持。
How
通过 C++17 的 chrono 绑定实现。