bcc64x needs ACE_HAS_TIME_T_LONG_MISMATCH, moved the implementation f…#2434
bcc64x needs ACE_HAS_TIME_T_LONG_MISMATCH, moved the implementation f…#2434jwillemsen wants to merge 3 commits into
Conversation
…or that out of inline so that the compiler can't optimize ACE_Time_Value::zero
* ACE/ace/Time_Value.cpp:
* ACE/ace/Time_Value.inl:
* ACE/ace/config-win32-borland.h:
WalkthroughThe changes introduce conditional handling for platforms with a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…peration will try to modify the const static object which causes an access violation with clang
* ACE/ace/Time_Value.cpp:
* ACE/ace/Time_Value.h:
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ACE/ace/Time_Value.h (1)
68-72: Consider adding explanatory comment for consistency.The conditional removal of
constformax_timefollows the same pattern as thezeromember, which is correct. However, consider adding an explanatory comment similar to lines 53-54 for thezeromember to maintain documentation consistency.#if defined (ACE_HAS_TIME_T_LONG_MISMATCH) + // Same const cast issue as with zero - avoid access violations with clang optimizations static ACE_Time_Value max_time; #else static const ACE_Time_Value max_time; #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ACE/ace/Time_Value.cpp(1 hunks)ACE/ace/Time_Value.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ACE/ace/Time_Value.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: jwillemsen
PR: DOCGroup/ACE_TAO#870
File: ACE/ace/UNIX_Addr.inl:39-39
Timestamp: 2025-02-11T10:00:08.023Z
Learning: In ACE_UNIX_Addr's equality operator, the comparison length should be sizeof(sun_path) - 1 to exclude the null terminator, not i. For Linux abstract paths, i is used to skip the first character (i=1) when both paths are empty, but the -1 is still needed to handle the null terminator correctly.
Learnt from: jwillemsen
PR: DOCGroup/ACE_TAO#870
File: ACE/ace/UNIX_Addr.inl:39-39
Timestamp: 2025-02-11T10:00:08.023Z
Learning: In ACE_UNIX_Addr's equality operator, the comparison length should be sizeof(sun_path) - 1 to exclude the null terminator. For Linux abstract paths, while i=1 is used to skip the first character, using i instead of -1 in the length calculation would be incorrect as it would either include the null terminator (for regular paths) or miss the last valid character (for abstract paths).
Learnt from: likema
PR: DOCGroup/ACE_TAO#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
PR: DOCGroup/ACE_TAO#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.
Learnt from: likema
PR: DOCGroup/ACE_TAO#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'.
⏰ 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). (4)
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: MINGW64-gcc
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: windows-2022 vs2022
🔇 Additional comments (1)
ACE/ace/Time_Value.h (1)
52-58: LGTM: Proper handling of const cast issues with compiler optimizations.The conditional removal of the
constqualifier for thezerostatic member whenACE_HAS_TIME_T_LONG_MISMATCHis defined correctly addresses the const cast access violation issue with clang optimizations. The approach maintains backward compatibility while solving the specific compiler problem.
|
Replaced by #2436 |
1 participant
…or that out of inline so that the compiler can't optimize ACE_Time_Value::zero
Summary by CodeRabbit