Add new compiler test to reproduce issue#2389
Conversation
* ACE/tests/Compiler_Features_42_Test.cpp:
Added.
* ACE/tests/run_test.lst:
* ACE/tests/tests.mpc:
* ACE/tests/Compiler_Features_42_Test.cpp:
* TAO/tao/AnyTypeCode/TypeCode_Case_T.h:
* ACE/tests/Compiler_Features_42_Test.cpp:
WalkthroughA new test, Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (10)
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
🧹 Nitpick comments (1)
ACE/tests/Compiler_Features_42_Test.cpp (1)
24-24: Consider using INT32_MIN for better readability.The current initialization
(-2147483647 - 1)is verbose. UsingINT32_MINfrom<climits>would be clearer.- int32_t disc_{ (-2147483647 - 1) }; + int32_t disc_{ INT32_MIN };Add
#include <climits>if not already included transitively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ACE/tests/Compiler_Features_42_Test.cpp(1 hunks)ACE/tests/run_test.lst(1 hunks)ACE/tests/tests.mpc(1 hunks)TAO/tao/AnyTypeCode/TypeCode_Case_T.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: alpine-3.18
- GitHub Check: MINGW64-gcc
- GitHub Check: windows-2019 vs2019
- GitHub Check: ubuntu-22.04 g++-12
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: ubuntu-24.04 g++-10 CodeQL
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: macos-14-C++
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: macos-13-C++
🔇 Additional comments (6)
TAO/tao/AnyTypeCode/TypeCode_Case_T.h (1)
39-39: LGTM! Comment formatting improvement.The line merge improves readability of the comment without affecting functionality.
ACE/tests/run_test.lst (1)
115-115: LGTM! Test properly registered in execution list.The new test is correctly positioned in the sequence and follows the established naming convention.
ACE/tests/tests.mpc (1)
866-871: LGTM! Build configuration follows established pattern.The project definition is consistent with other compiler feature tests and includes all necessary components.
ACE/tests/Compiler_Features_42_Test.cpp (3)
25-31: Union with non-trivial types requires careful management.The union contains both
std::array<std::string, 5>andint32_t. Sincestd::stringhas non-trivial constructors/destructors, proper union management typically requires:
- A discriminator to track the active member
- Proper construction/destruction of the active member
- Manual lifetime management
The current implementation with empty destructors (lines 35-40) may not properly destruct the
arrayofstringmember, potentially causing resource leaks.However, since this test is designed to reproduce a specific VS2022 compiler issue, this pattern might be intentional. Please verify that this union structure correctly reproduces the intended compiler behavior.
35-40: Empty destructors may cause resource leaks.The union and class destructors are empty, which means:
- If the union's
as_member (array of strings) is active when destroyed, the strings won't be properly destructed- This could lead to resource leaks in production code
Since this is a compiler issue reproduction test, this might be intentional to trigger the specific VS2022 issue being tested.
Please confirm this destructor behavior is required to reproduce the compiler issue. If not, consider proper union member destruction based on the discriminator value.
46-47: Test successfully demonstrates basic object lifecycle.The test correctly allocates and deallocates an instance of the problematic class, which should be sufficient to reproduce the compiler issue if it exists.
* ACE/tests/Compiler_Features_42_Test.cpp:
jwillemsen
changed the title
* ACE/tests/Compiler_Features_42_Test.cpp:
…to jwi-vs2022bug
* ACE/tests/tests.mpc:
* ACE/tests/tests.mpc:
|
Enabling C++20 used as workaround, issue has been reported to Microsoft. Visual Studio 2019 also gives an internal compiler error on the example code |
1 participant
Summary by CodeRabbit