ACE_Time_Value Should Accept Floating-point-based std::chrono::duration#2462
Conversation
WalkthroughACE_Time_Value::set now computes seconds via duration_cast and microseconds from duration - seconds; operator+= and operator-= delegate to ACE_Time_Value(duration). Chrono tests were centralized into a tv_test_case helper and extended to cover floating-point durations. tests/.gitignore entries were reorganized. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Chrono as std::chrono::duration
participant ACE as ACE_Time_Value
Note over Caller,Chrono: Input: duration (Rep,Period)
Caller->>ACE: ACE_Time_Value::set(duration)
ACE->>Chrono: duration_cast<std::chrono::seconds>(duration)
Chrono-->>ACE: sec (seconds)
ACE->>Chrono: remainder = duration - sec
Chrono-->>ACE: remainder (duration)
ACE->>Chrono: duration_cast<std::chrono::microseconds>(remainder)
Chrono-->>ACE: usec (microseconds)
ACE->>ACE: truncate_cast(sec.count()), truncate_cast(usec.count())
ACE-->>Caller: constructed ACE_Time_Value(sec,usec)
Note right of ACE: operator+= / operator-= delegate to\nACE_Time_Value(duration) then reuse existing ops
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ACE/tests/Chrono_Test.cpp (1)
147-158: Good positive FP-duration coverage; add negative-duration cases to lock semantics.Please add tests for:
- negative fractional day (e.g., -0.5 days → sec = -43200, usec = 0);
- small negative fractional seconds (e.g., -0.1s → sec = -1, usec = 900000) to verify canonicalization.
Example:
if (tv.sec () != 3600*12 || tv.usec () != 0) { ... } + + // Negative floating-point durations should canonicalize. + std::chrono::duration<double, std::ratio<86400>> const neg_half_day {-0.5}; + tv = ACE_Time_Value { neg_half_day }; + if (tv.sec () != -3600*12 || tv.usec () != 0) + { + ACE_ERROR ((LM_ERROR, + ACE_TEXT ("(%P|%t) unexpected value after converting ") + ACE_TEXT ("-0.5 day to ACE_Time_Value. <sec=-43200,usec=0> - ") + ACE_TEXT ("got <sec=%d,usec=%d>\n"), tv.sec (), tv.usec ())); + ++errors; + } + + std::chrono::duration<double> const neg_tenth {-0.1}; + tv = ACE_Time_Value { neg_tenth }; + if (tv.sec () != -1 || tv.usec () != 900000) + { + ACE_ERROR ((LM_ERROR, + ACE_TEXT ("(%P|%t) unexpected value after converting ") + ACE_TEXT ("-0.1s to ACE_Time_Value. <sec=-1,usec=900000> - ") + ACE_TEXT ("got <sec=%d,usec=%d>\n"), tv.sec (), tv.usec ())); + ++errors; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ACE/ace/Time_Value.h(1 hunks)ACE/tests/.gitignore(5 hunks)ACE/tests/Chrono_Test.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: VS2022Debug64
- GitHub Check: ubuntu-24.04 g++-10 wchar
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-24.04 clang++-20 c++23
- GitHub Check: ubuntu-24.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-24.04 clang++-20
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-22.04 clang++-12
🔇 Additional comments (5)
ACE/tests/.gitignore (5)
97-103: Confirm intent of Compiler_Features ignore set change.Dropping 36–38 and adding 39–42 will start tracking 36–38 if those dirs exist. If that’s intended (e.g., tests stabilized), fine; otherwise, keep them ignored for consistency.
120-120: LGTM: add Future_Stress_Test to ignore.No issues.
163-163: Multicast_Interfaces_Test ignore entry changed—verify desired state.Network-interface dependent tests often produce local flakiness/noise. Ensure we either keep ignoring it or add CI guards before unignoring.
229-231: Singleton_Test/SOCK_Acceptor_Test entries—double-check policy.If these tests are meant to be tracked, remove from .gitignore; if not, keep them here. Current change suggests policy shift—please confirm.
270-270: LGTM: add TSS_Static_Test to ignore.Matches typical policy for platform-dependent TSS tests.
`ACE_Time_Value` currently doesn't accept a `std::chrono::duration` with `Rep` that's a floating point type because it's using the modulus operator to separate the microseconds (`duration % seconds(1)`) and trying to use modulus with floating points causes a compile error. This changes it to use subtraction from the whole number of seconds (`duration - sec`) and adds a test for it. Also: - Simplify the `-=` and `+=` ops of `ACE_Time_Value` - Rewrite some of the testing in `Chrono_Test.cpp`.
iguessthislldo
force-pushed
the
igtd/fp-duration-to-time-value
branch
from
222db9e to
b226740
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ACE/ace/Time_Value.h (1)
121-127: Float-duration remainder extraction fix is correct; optional: canonicalize negative remainders.Subtraction avoids the FP modulo compile error. If you want usec in [0..1s) canonical form (not required by current tests), adjust negative remainders locally before calling set(). This mirrors “seconds-minus-one, remainder-plus-one-second.”
using std::chrono::seconds; using std::chrono::microseconds; using std::chrono::duration_cast; - auto const sec = duration_cast<seconds> (duration); - auto const usec = duration_cast<microseconds> (duration - sec); - this->set (ACE_Utils::truncate_cast<time_t> (sec.count ()), ACE_Utils::truncate_cast<suseconds_t> (usec.count ())); + seconds sec = duration_cast<seconds>(duration); + auto rem = duration - sec; + if (rem < seconds{0}) { + sec -= seconds{1}; + rem += seconds{1}; + } + auto const usec = duration_cast<microseconds>(rem); + this->set(ACE_Utils::truncate_cast<time_t>(sec.count()), + ACE_Utils::truncate_cast<suseconds_t>(usec.count()));
🧹 Nitpick comments (4)
ACE/tests/.gitignore (1)
97-103: Verify intent of Compiler_Features ignore list changes.You added 39–42 and still list 36–38. If 36–38 are meant to be tracked (unignored), please remove them; if they should remain ignored, keep all consistently. Consider keeping this section sorted to reduce future diffs.
ACE/tests/Chrono_Test.cpp (3)
24-35: Use safe formatter for time_t in logs.On platforms where time_t is 64-bit, %d is unsafe. Use %: for time_t in ACE logging.
- ACE_TEXT ("Expected <sec=%d,usec=%d> - got <sec=%d,usec=%d>\n"), - what, expect_sec, expect_usec, tv.sec (), tv.usec ())); + ACE_TEXT ("Expected <sec=%:,usec=%d> - got <sec=%:,usec=%d>\n"), + what, expect_sec, expect_usec, tv.sec (), tv.usec ()));
307-315: Fix expected values in error messages (typos only).Messages still say 3300 us; the checks expect 303000 and 302600.
- ACE_TEXT ("of 300 ms. Expected <sec=2,usec=3300> - got <sec=%d,") + ACE_TEXT ("of 300 ms. Expected <sec=2,usec=303000> - got <sec=%d,")- ACE_TEXT ("of 400 us. Expected <sec=2,usec=3300> - got <sec=%d,") + ACE_TEXT ("of 400 us. Expected <sec=2,usec=302600> - got <sec=%d,")Also applies to: 321-324
352-355: Spelling: “substracting” → “subtracting”.Update log messages for clarity.
- ACE_TEXT ("unexpected value after substracting an ACE_Time_Value ") + ACE_TEXT ("unexpected value after subtracting an ACE_Time_Value ")(Apply to all occurrences in these ranges.)
Also applies to: 376-382, 400-405, 424-429, 471-476
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
ACE/NEWS(1 hunks)ACE/ace/Time_Value.h(3 hunks)ACE/tests/.gitignore(5 hunks)ACE/tests/Chrono_Test.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: VS2022Release64
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Debug64Cxx17
- GitHub Check: VS2022Release32
- GitHub Check: MINGW64-gcc
- GitHub Check: ubuntu-24.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-24.04 clang++-20 c++23
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 clang++-20
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: alpine-3.18
🔇 Additional comments (8)
ACE/tests/.gitignore (4)
120-120: Confirm ignoring Future_Stress_Test.Stress tests are often excluded from default runs. If CI relies on this being ignored, LGTM; otherwise, consider enabling it for periodic runs.
163-163: Multicast_Interfaces_Test ignore status changed — confirm.This test can be platform/network dependent. Please confirm the change aligns with CI environments.
229-231: Singleton_Test and SOCK_Acceptor_Test ignore changes.SOCK_Acceptor_Test has historically had intermittent platform differences. Please confirm this change is intentional and CI-ready.
270-270: TSS_Static_Test ignore status — OK.No concerns; consistent with other TSS tests’ handling.
ACE/ace/Time_Value.h (2)
271-276: Delegating +=(duration) to +=(ACE_Time_Value) — good simplification.Reduces duplication and centralizes normalization. LGTM.
286-291: Delegating -=(duration) to -=(ACE_Time_Value) — good simplification.Consistent with += path; no issues.
ACE/tests/Chrono_Test.cpp (1)
111-112: Nice coverage for mixed-unit rounding.The 1120ms case validates microsecond carry correctly. LGTM.
ACE/NEWS (1)
4-5: NEWS entry is concise and accurate.Matches the code changes and tests. LGTM.
This is a backport of DOCGroup#2462 `ACE_Time_Value` currently doesn't accept a `std::chrono::duration` with `Rep` that's a floating point type because it's using the modulus operator to separate the microseconds (`duration % seconds(1)`) and trying to use modulus with floating points causes a compile error. This changes it to use subtraction from the whole number of seconds (`duration - sec`) and adds a test for it. Also: - Simplify the `-=` and `+=` ops of `ACE_Time_Value` - Rewrite some of the testing in `Chrono_Test.cpp`.
3 participants
ACE_Time_Valuecurrently doesn't accept astd::chrono::durationwithRepthat's a floating point type because it's using the modulus operator to separate the microseconds (duration % seconds(1)) and trying to use modulus with floating points causes a compile error.This changes it to use subtraction from the whole number of seconds (
duration - sec) and adds a test for it.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores