feat!: changed time to unsigned 32-bit preserving ltime as 64-bit#1801
feat!: changed time to unsigned 32-bit preserving ltime as 64-bit#1801Angus-Bethke-Bachmann wants to merge 9 commits into
Conversation
Build Artifacts🪟 Windows
From workflow run 🐧 Linux
From workflow run |
ghaith
changed the title
|
since this is a beaking change, i updated the title to reflect that ( |
volsa
left a comment
volsa
left a comment
There was a problem hiding this comment.
Focused review — correctness and test-coverage only (skipping style/nits). Overall this is a solid, comprehensive breaking change and CI is green across x86_64/aarch64/Windows. A few correctness/semantics points and test gaps below that are worth a decision before merge.
| pub extern "C-unwind" fn SUB_DATE_DATE(in1: i64, in2: i64) -> u32 { | ||
| let lhs = short_date_time_seconds(in1); | ||
| let rhs = short_date_time_seconds(in2); | ||
| checked_seconds_to_millis(lhs.wrapping_sub(rhs)) |
There was a problem hiding this comment.
Correctness — silent overflow for large date differences.
SUB_DATE_DATE/SUB_DT_DT return a TIME (u32 ms), which tops out at ~49.7 days. checked_seconds_to_millis uses wrapping_mul, so any date difference beyond that silently wraps to a garbage duration — no panic, no diagnostic. Previously this was i64 nanoseconds (~292-year range).
The wrapping_* choice looks intentional (the datetime_arithmetic_time.st lit test asserts -3000 for SUB_TIME underflow), but two issues remain:
- The doc comments on these short add/sub ops still say "Panic on overflow", which is now inaccurate.
- Large-diff overflow is untested.
Suggest documenting the ~49.7-day ms range limit (ideally saturate or validate instead of wrapping) and fixing the stale doc comments.
| .unwrap() | ||
| .num_nanoseconds() | ||
| .unwrap() | ||
| pub extern "C-unwind" fn ADD_TIME(in1: i64, in2: i64) -> u32 { |
There was a problem hiding this comment.
FFI signature mismatch (short variants).
The ST declaration is now FUNCTION ADD_TIME: TIME … IN1/IN2: TIME (32-bit), so the compiler emits this call with i32 arguments, while the Rust definition takes i64. It only works because short_time_millis masks with as u32, discarding the unspecified upper register bits — CI is green, so it holds on x86_64/aarch64/Windows — but it's an ABI mismatch that relies on register-passing coincidence and reads misleadingly (an i64 param that is really a u32).
Recommend changing the short-variant parameters to u32/i32 to match the ST-declared types (and dropping the now-redundant short_time_millis/short_date_time_seconds casts). Applies to all the short ADD_*/SUB_* functions in this file.
| name: TIME_TYPE.into(), | ||
| signed: true, | ||
| size: DATE_TIME_SIZE, | ||
| signed: false, |
There was a problem hiding this comment.
Semantics — short TIME/TOD/DATE/DT are now unsigned.
With signed: false, negative durations no longer behave: T#1S - T#2S goes through wrapping_sub to ~4.29e9 ms, and comparisons/divisions on TIME are emitted as unsigned. IEC 61131-3 permits negative TIME.
The unit tests confirm this is a real behavior change — the old negative short-TIME cases (ADD_TIME(TIME#10s, TIME#-5s), SUB(TIME#10h50m, TIME#-10m)) were removed and migrated to LTIME. Is dropping negative short TIME intended? If so, should a negative short-TIME literal be rejected by validation rather than silently wrapping to a large positive value?
| // SUB_DATE_DATE: DATE - DATE -> TIME (u32) | ||
| date1 := DATE#2024-01-15; | ||
| date2 := DATE#2024-01-10; | ||
| result := SUB_DATE_DATE(date1, date2); |
There was a problem hiding this comment.
Test gap.
This covers a 5-day difference (fits in u32 ms), but there's no case for a difference beyond ~49.7 days — exactly where SUB_DATE_DATE silently wraps (wrapping_mul in checked_seconds_to_millis). Worth adding a large-diff case to pin down the intended overflow behavior.
| a := ADD(TIME#5h,TIME#30s); | ||
| b := ADD_TIME(TIME#10s,TIME#-5s); | ||
| a := ADD(LTIME#5h,LTIME#30s); | ||
| b := ADD_LTIME(LTIME#10s,LTIME#-5s); |
There was a problem hiding this comment.
Test gap — short-type arithmetic coverage moved to long variants.
This add_time test (and sub_time, sub_date, add_tod_time, add_dt_time, sub_tod_time, …) were all switched from the short types to LTIME/LDATE/LTOD/LDT. As a result the rewritten short-type paths — the new millisecond/second representation, unsigned handling, and wrapping_* overflow — are no longer exercised by these unit tests (only a couple of tests/lit/... cases cover them), and the negative short-TIME case that used to live here (ADD_TIME(TIME#10s, TIME#-5s)) is gone.
Consider keeping short-type variants of these tests alongside the new long ones so the 32-bit ms/second behavior stays covered at the unit level.
3 participants
Please Note: Merge should be delayed until downstream dependent applications are prepared. This is a breaking change!
Changed
TIME,TIME_OF_DAY/TOD,DATEandDATE_AND_TIME/DTare now unsigned 32-bit.LTIME,LTIME_OF_DAY/LTOD,LDATEandLDATE_AND_TIME/LDTremain 64-bit.TIMEandTIME_OF_DAY/TODare represented as milliseconds.DATEandDATE_AND_TIME/DTare represented as seconds.Documentation
Testing
PS: This is my first largely agent assisted PR, please take a look at it carefully. I found quite a few places where it had made assumptions I believed to be incorrect during my self-review, hopefully it was all of them.