fix a bug in the ACE log mechanism when using a '%#T' format string t… #2484
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…o print time periods. These logging calls invoke ACE::timestamp, which in turn (erroneously) invokes localtime_r (instead of gmtime_r). Localtime(_r) always adds the timezone offset (if any) to the hour field, which results in a wrong result string (when you live anywhere outside of the GMT time zone)
Contributor
WalkthroughAdded ACE::duration to format ACE_Time_Value durations as "hour:minute:second.microsecond." and declared it public; integrated a new log-format specifier Changes
Sequence Diagram(s)sequenceDiagram
participant Formatter as Log_Msg formatter
participant ACE as ACE utility
participant Out as Output buffer
Formatter->>Formatter: parse format string
alt "%Y" with '#' modifier
Formatter->>ACE: ACE::duration(duration_value, buf, len)
ACE-->>Formatter: formatted duration string
Formatter->>Out: append formatted duration (update lengths)
else "%Y" without '#'
Formatter->>Formatter: emit error / fallback handling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/ace/ACE.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: iguessthislldo
Repo: DOCGroup/ACE_TAO PR: 2462
File: ACE/ace/Time_Value.h:0-0
Timestamp: 2025-09-03T01:30:16.319Z
Learning: In ACE_Time_Value, the set(time_t, suseconds_t) method automatically calls normalize() which handles negative microsecond values by adjusting seconds and microseconds into canonical form, so explicit normalization in template methods that call set() is unnecessary.
Learnt from: iguessthislldo
Repo: DOCGroup/ACE_TAO PR: 2462
File: ACE/ace/Time_Value.h:0-0
Timestamp: 2025-09-03T01:30:16.319Z
Learning: The ACE_Time_Value::set(time_t, suseconds_t) method normalizes negative microsecond values, so explicit normalization in template methods that call set is redundant.
📚 Learning: 2025-09-03T01:30:16.319Z
Learnt from: iguessthislldo
Repo: DOCGroup/ACE_TAO PR: 2462
File: ACE/ace/Time_Value.h:0-0
Timestamp: 2025-09-03T01:30:16.319Z
Learning: The ACE_Time_Value::set(time_t, suseconds_t) method normalizes negative microsecond values, so explicit normalization in template methods that call set is redundant.
Applied to files:
ACE/ace/ACE.cpp
📚 Learning: 2025-09-03T01:30:16.319Z
Learnt from: iguessthislldo
Repo: DOCGroup/ACE_TAO PR: 2462
File: ACE/ace/Time_Value.h:0-0
Timestamp: 2025-09-03T01:30:16.319Z
Learning: In ACE_Time_Value, the set(time_t, suseconds_t) method automatically calls normalize() which handles negative microsecond values by adjusting seconds and microseconds into canonical form, so explicit normalization in template methods that call set() is unnecessary.
Applied to files:
ACE/ace/ACE.cpp
🧬 Code graph analysis (1)
ACE/ace/ACE.cpp (1)
ACE/ace/OS_NS_time.cpp (1)
localtime_r(17-17)
⏰ 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: macos-13-C++
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-24.04 clang++-20
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 clang++-20 c++23
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-24.04 g++-10 CodeQL
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-24.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-24.04 g++-10 wchar
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: MINGW64-gcc
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: alpine-3.18
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
ACE/ace/ACE.h (1)
480-518: timestamp/duration API docs align with implementation; consider clarifying 24h limitation wording
- The timestamp comment now explicitly calls out the
ACE_Time_Value::zero→gettimeofday()behavior, which matches the implementation inACE::timestampand is helpful for callers.- The new
duration(const ACE_Time_Value&, ACE_TCHAR*, size_t)API is documented with a minimum buffer size and note about durations ≥ 24 hours; this matches the formatting width and the fact that onlytm_hour/min/secare printed.If you want to be more explicit, you could slightly rephrase “cannot handle durations larger or equal to 24 hours” to mention that the hours field effectively wraps at 24h (as also hinted by the
%Ydocs), but it’s not required for correctness.ACE/ace/Log_Msg.h (1)
545-562: Logging format docs for %T/%Y are consistent; you may want to tighten the %Y contract
- Updating
%Ttohour:minute:sec.usecwith#expectingACE_Time_Value*matches the actualACE::timestampbehavior.- The new
%Ydescription correctly documents the duration use case and the ≥24h overflow caveat, in line withACE::duration.Given that durations really only make sense with an explicit
ACE_Time_Value*, you might consider saying that plain%Yis “unsupported/invalid” rather than “currently unspecified”, to make the intended usage (%#Y) unambiguous.ACE/ace/ACE.cpp (1)
2433-2464: ACE::duration implementation correctly fixes the timezone bug for durations
- The new
ACE::durationusesgmtime_ron the duration seconds and printsHH:MM:SS.uuuuuu, which avoids applying any local timezone offset—this directly addresses the original%#T/time-period bug.- The
duration_len < 16guard matches the required fixed-width output and returnsEINVALconsistently with other ACE time-format helpers.- The behavior for very long or negative durations is limited (wrap modulo 24h / odd values), but this is already called out in the surrounding documentation and
%Ydescription, so it’s an acceptable constraint for this API.If you later need to support durations ≥24h or signed durations more explicitly, you could extend this helper (e.g., by returning an error for out-of-range values or formatting days separately), but that’s not necessary for this bugfix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ACE/ace/ACE.cpp(2 hunks)ACE/ace/ACE.h(2 hunks)ACE/ace/Log_Msg.cpp(2 hunks)ACE/ace/Log_Msg.h(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 2332
File: ACE/ace/Ping_Socket.cpp:227-232
Timestamp: 2025-02-13T18:50:43.028Z
Learning: In ACE framework's logging system (ACE_Log_Msg::log), `%b` is the correct format specifier for printing ssize_t values, not the standard C++ `%zd`.
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 2332
File: ACE/ace/Ping_Socket.cpp:270-277
Timestamp: 2025-02-13T18:51:40.093Z
Learning: In ACE_Log_Msg logging system, '%b' is the correct format specifier for ssize_t values, not '%zd'. This is a custom format specifier specific to ACE's logging system.
📚 Learning: 2025-02-13T18:51:40.093Z
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 2332
File: ACE/ace/Ping_Socket.cpp:270-277
Timestamp: 2025-02-13T18:51:40.093Z
Learning: In ACE_Log_Msg logging system, '%b' is the correct format specifier for ssize_t values, not '%zd'. This is a custom format specifier specific to ACE's logging system.
Applied to files:
ACE/ace/Log_Msg.cppACE/ace/ACE.cpp
📚 Learning: 2025-02-13T18:50:43.028Z
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 2332
File: ACE/ace/Ping_Socket.cpp:227-232
Timestamp: 2025-02-13T18:50:43.028Z
Learning: In ACE framework's logging system (ACE_Log_Msg::log), `%b` is the correct format specifier for printing ssize_t values, not the standard C++ `%zd`.
Applied to files:
ACE/ace/Log_Msg.cppACE/ace/ACE.cpp
📚 Learning: 2025-09-03T01:30:16.319Z
Learnt from: iguessthislldo
Repo: DOCGroup/ACE_TAO PR: 2462
File: ACE/ace/Time_Value.h:0-0
Timestamp: 2025-09-03T01:30:16.319Z
Learning: In ACE_Time_Value, the set(time_t, suseconds_t) method automatically calls normalize() which handles negative microsecond values by adjusting seconds and microseconds into canonical form, so explicit normalization in template methods that call set() is unnecessary.
Applied to files:
ACE/ace/ACE.cpp
📚 Learning: 2025-09-03T01:30:16.319Z
Learnt from: iguessthislldo
Repo: DOCGroup/ACE_TAO PR: 2462
File: ACE/ace/Time_Value.h:0-0
Timestamp: 2025-09-03T01:30:16.319Z
Learning: The ACE_Time_Value::set(time_t, suseconds_t) method normalizes negative microsecond values, so explicit normalization in template methods that call set is redundant.
Applied to files:
ACE/ace/ACE.cpp
📚 Learning: 2025-02-13T18:51:40.093Z
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 2332
File: ACE/ace/Ping_Socket.cpp:270-277
Timestamp: 2025-02-13T18:51:40.093Z
Learning: In ACE_Log_Msg logging system, '%b' is the correct format specifier to print ssize_t values, not '%zd'.
Applied to files:
ACE/ace/ACE.cpp
🧬 Code graph analysis (2)
ACE/ace/ACE.h (1)
ACE/ace/ACE.cpp (2)
duration(2437-2464)duration(2438-2440)
ACE/ace/Log_Msg.cpp (2)
ACE/ace/ACE.cpp (2)
duration(2437-2464)duration(2438-2440)ACE/ace/Time_Value.h (1)
ACE_Time_Value(48-128)
⏰ 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: VS2022Debug32
- GitHub Check: VS2022Release64
- GitHub Check: VS2022Debug64Cxx17
- GitHub Check: VS2022WChar
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Debug64Cxx20
- GitHub Check: VS2022Release32
- GitHub Check: MINGW64-gcc
- GitHub Check: macos-14-C++
- GitHub Check: macos-13-C++
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-24.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: windows-2022 vs2022
- GitHub Check: ubuntu-24.04 g++
🔇 Additional comments (1)
ACE/ace/ACE.cpp (1)
2391-2431: timestamp comment now accurately reflects behavior; no functional changeThe only change here is the comment text (colon → dot before microseconds); the implementation still uses
localtime_rand theACE_Time_Value::zero→gettimeofday()path, so existing%Tbehavior remains intact and the docs are now aligned with the actual format string.
Contributor
Author
|
AFAICT the CodeFactor issue is already addressed in the code and can be safely ignored |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…o print time periods.
These logging calls invoke ACE::timestamp, which in turn (erroneously) invokes localtime_r (instead of gmtime_r). Localtime(_r) always adds the timezone offset (if any) to the hour field, which results in a wrong result string (when you live anywhere outside of the GMT time zone)
Summary by CodeRabbit
New Features
Documentation
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.