Add back casts for msvc, only the const cast was not necessary #2437
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
* ACE/ace/Time_Value.inl:
WalkthroughThe conversion operators in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
✨ 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 (
|
![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
♻️ Duplicate comments (1)
ACE/ace/Time_Value.inl (1)
42-46: Same truncation concern applies here.The pointer conversion overload duplicates the truncation logic; please apply the same range-check (or extract a small helper) to avoid divergence between the two code paths.
🧹 Nitpick comments (1)
ACE/ace/Time_Value.inl (1)
12-15: Validate potential precision-loss when truncatingtv_sec/tv_usectolong.
ACE_Utils::truncate_cast<long>is a good step to silence the MSVC mismatch and make the narrowing explicit, but it will still silently clip values outsideLONG_MIN … LONG_MAX.
Becausetime_tis 64-bit on 64-bit Windows whilelongis 32-bit, anACE_Time_Valuefarther than ~24855 days from the epoch will wrap.Consider guarding the cast with an ACE assertion in debug builds so accidental overflow is caught early:
#if defined (ACE_HAS_TIME_T_LONG_MISMATCH) +# if defined (ACE_NDEBUG) + // Fast path – no extra checks in release builds. this->ext_tv_.tv_sec = ACE_Utils::truncate_cast<long> (this->tv_.tv_sec); this->ext_tv_.tv_usec = ACE_Utils::truncate_cast<long> (this->tv_.tv_usec); +# else + ACE_ASSERT (this->tv_.tv_sec <= LONG_MAX && this->tv_.tv_sec >= LONG_MIN); + ACE_ASSERT (this->tv_.tv_usec <= LONG_MAX && this->tv_.tv_usec >= LONG_MIN); + this->ext_tv_.tv_sec = static_cast<long> (this->tv_.tv_sec); + this->ext_tv_.tv_usec = static_cast<long> (this->tv_.tv_usec); +# endifThis keeps the release build fast while surfacing dangerous ranges during testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/ace/Time_Value.inl(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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: 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: 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: 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). (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: VS2022Release64
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Release32
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Debug64Cxx17
- GitHub Check: VS2022Debug64Cxx20
- GitHub Check: VS2022Debug32
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: windows-2022 vs2022
- GitHub Check: macos-13-C++
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: macos-14-C++
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit