Add new unit test for runtime exceptions#2505
Conversation
* ACE/tests/Compiler_Features_43_Test.cpp:
Added.
* ACE/tests/run_test.lst:
* ACE/tests/tests.mpc:
WalkthroughAdds a new ACE test that exercises DynAny/DynValue recursive creation, indirection via thrown pointers, and lifecycle logging; registers the test in ACE test lists/build, and updates a CI vcpkg commit reference. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ACE/tests/Compiler_Features_43_Test.cpp (2)
94-97: UseACE_ERRORfor error-severity messages.
ACE_DEBUGpaired withLM_ERRORis non-idiomatic in ACE code;ACE_ERRORis the conventional macro for error-priority log output.✏️ Suggested fix
- ACE_DEBUG ((LM_ERROR, ACE_TEXT("In outer catch\n"))); + ACE_ERROR ((LM_ERROR, ACE_TEXT("In outer catch\n")));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ACE/tests/Compiler_Features_43_Test.cpp` around lines 94 - 97, Replace the non-idiomatic ACE_DEBUG((LM_ERROR, ...)) call in the outer catch (...) block with the conventional ACE_ERROR macro; locate the catch (...) block that sets res = -1 and replace the ACE_DEBUG usage (referenced as ACE_DEBUG and the variable res in the outer catch) so the error message is emitted via ACE_ERROR with the same text ("In outer catch\n").
41-42: "destroying blank" log label is inaccurate.By the time the
DA_IMPL*is caught,phas already executedfrom_inputCDRand may have populatedda_members_; calling it "blank" is misleading. Consider "destroying partial" or "destroying wrapper" to accurately reflect its state.✏️ Suggested log wording
- ACE_DEBUG ((LM_DEBUG, ACE_TEXT("!!! CAUGHT pointer %@ at depth %d (destroying blank %@) !!!\n"), + ACE_DEBUG ((LM_DEBUG, ACE_TEXT("!!! CAUGHT pointer %@ at depth %d (destroying partial wrapper %@) !!!\n"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ACE/tests/Compiler_Features_43_Test.cpp` around lines 41 - 42, The log message incorrectly calls the caught DA_IMPL* instance "blank" even though p may have had from_inputCDR run and populated da_members_; update the debug text in the ACE_DEBUG call that references original, depth and p to a more accurate label such as "destroying partial" or "destroying wrapper" so it reflects the instance state (use the ACE_DEBUG call surrounding the catch of DA_IMPL*, referencing original, depth, p and from_inputCDR/da_members_ in your change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ACE/tests/Compiler_Features_43_Test.cpp`:
- Around line 55-68: da_members_ currently holds raw void* (in from_inputCDR
where CreateDynAnyUtils<TAO_DynValue_i>::create stores members), causing leaked
objects when owners like p0 are destroyed (p1/p3 leaked); fix by making
ownership explicit: change da_members_ to store TAO_DynValue_i* (or better
std::unique_ptr<TAO_DynValue_i>), and ensure the owning class (where
from_inputCDR is defined) deletes or lets unique_ptr clean up members in its
destructor, or if non-owning pointers are intended, add a clear comment above
da_members_ and in run_main explaining deliberate omission so leak detectors
won’t be confused.
---
Nitpick comments:
In `@ACE/tests/Compiler_Features_43_Test.cpp`:
- Around line 94-97: Replace the non-idiomatic ACE_DEBUG((LM_ERROR, ...)) call
in the outer catch (...) block with the conventional ACE_ERROR macro; locate the
catch (...) block that sets res = -1 and replace the ACE_DEBUG usage (referenced
as ACE_DEBUG and the variable res in the outer catch) so the error message is
emitted via ACE_ERROR with the same text ("In outer catch\n").
- Around line 41-42: The log message incorrectly calls the caught DA_IMPL*
instance "blank" even though p may have had from_inputCDR run and populated
da_members_; update the debug text in the ACE_DEBUG call that references
original, depth and p to a more accurate label such as "destroying partial" or
"destroying wrapper" so it reflects the instance state (use the ACE_DEBUG call
surrounding the catch of DA_IMPL*, referencing original, depth, p and
from_inputCDR/da_members_ in your change).
| std::vector<void*> da_members_; | ||
| void from_inputCDR(bool trigger_indirection, int depth) { | ||
| ACE_DEBUG ((LM_DEBUG, ACE_TEXT(" from_inputCDR depth=%d this=%@\n"), depth, this)); | ||
| // Simulates deep recursion of ACE_TAO (exactly like in the real DynValue_i) | ||
| if (depth < 3) { | ||
| ACE_DEBUG ((LM_DEBUG, ACE_TEXT(" Creating nested member at depth %d\n"), depth + 1)); | ||
| try { | ||
| auto* member = CreateDynAnyUtils<TAO_DynValue_i>::create(trigger_indirection, depth + 1); | ||
| da_members_.push_back(member); | ||
| } | ||
| catch (TAO_DynValue_i* orig) { | ||
| ACE_DEBUG ((LM_DEBUG,ACE_TEXT(" Nested member threw indirection %@\n"), orig)); | ||
| da_members_.push_back(orig); | ||
| } |
There was a problem hiding this comment.
da_members_ stores raw void with no cleanup — p1 and p3 are leaked.*
When p0 is deleted in run_main, p0->da_members_ (holding p1 as void*) is destroyed without deleting the pointed-to object, because std::vector<void*> has no element destructor. p1 itself is therefore never freed, and p1->da_members_ (holding p3) leaks transitively. This is a simplified stand-in for the real TAO refcounting, but the stubs provide no _remove_ref/delete path, so LeakSanitizer/Valgrind will flag these as definite leaks.
If this is intentional, a comment noting the deliberate omission of cleanup would prevent false alarms and clarify intent for future readers.
🧰 Tools
🪛 Cppcheck (2.19.0)
[error] 61-61: #error ACE/TAO require C++17 compliance, please upgrade your compiler and/or fix the platform configuration for your environment
(preprocessorErrorDirective)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ACE/tests/Compiler_Features_43_Test.cpp` around lines 55 - 68, da_members_
currently holds raw void* (in from_inputCDR where
CreateDynAnyUtils<TAO_DynValue_i>::create stores members), causing leaked
objects when owners like p0 are destroyed (p1/p3 leaked); fix by making
ownership explicit: change da_members_ to store TAO_DynValue_i* (or better
std::unique_ptr<TAO_DynValue_i>), and ensure the owning class (where
from_inputCDR is defined) deletes or lets unique_ptr clean up members in its
destructor, or if non-owning pointers are intended, add a clear comment above
da_members_ and in run_main explaining deliberate omission so leak detectors
won’t be confused.
* .github/workflows/windows.yml:
* ACE/tests/Compiler_Features_43_Test.cpp:
* .github/workflows/windows.yml:
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ACE/tests/Compiler_Features_43_Test.cpp (1)
61-68: The innercatch (TAO_DynValue_i* orig)at line 65 is unreachable in this test's execution path.
CreateDynAnyUtils<TAO_DynValue_i>::create()internalises the thrown pointer — it catchesDA_IMPL*and returns it as a normal value (lines 40–45). Consequently, the nested call tocreate(...)on line 62 never propagates aTAO_DynValue_i*exception back tofrom_inputCDR, making thecatchon line 65 dead code. If this mirrors defensive logic from real TAO code, a short comment explaining the intended scenario would clarify intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ACE/tests/Compiler_Features_43_Test.cpp` around lines 61 - 68, The catch block catching TAO_DynValue_i* after calling CreateDynAnyUtils<TAO_DynValue_i>::create(...) inside from_inputCDR is unreachable because create() internalises DA_IMPL* exceptions and never propagates TAO_DynValue_i*; remove the dead catch (the catch (TAO_DynValue_i* orig) block) or, if you want to preserve the defensive pattern, replace it with a short clarifying comment above the try/catch (referencing CreateDynAnyUtils<TAO_DynValue_i>::create and from_inputCDR) explaining that production code internalises DA_IMPL* and this path is kept only for parity with TAO or defensive reasons so reviewers won’t be confused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ACE/tests/Compiler_Features_43_Test.cpp`:
- Around line 55-68: da_members_ is a std::vector<void*> causing ownership leaks
(p1/p3) when a parent like p0 is destroyed; update from_inputCDR and the
container to manage ownership correctly by replacing std::vector<void*>
da_members_ with a typed RAII container (e.g.,
std::vector<std::unique_ptr<TAO_DynValue_i>> or
std::vector<std::shared_ptr<TAO_DynValue_i>>) and push the returned pointer from
CreateDynAnyUtils<TAO_DynValue_i>::create into that smart pointer (and change
the catch branch to capture and store the TAO_DynValue_i* appropriately),
ensuring proper destruction in the class destructor/automatic cleanup instead of
raw void* storage.
---
Nitpick comments:
In `@ACE/tests/Compiler_Features_43_Test.cpp`:
- Around line 61-68: The catch block catching TAO_DynValue_i* after calling
CreateDynAnyUtils<TAO_DynValue_i>::create(...) inside from_inputCDR is
unreachable because create() internalises DA_IMPL* exceptions and never
propagates TAO_DynValue_i*; remove the dead catch (the catch (TAO_DynValue_i*
orig) block) or, if you want to preserve the defensive pattern, replace it with
a short clarifying comment above the try/catch (referencing
CreateDynAnyUtils<TAO_DynValue_i>::create and from_inputCDR) explaining that
production code internalises DA_IMPL* and this path is kept only for parity with
TAO or defensive reasons so reviewers won’t be confused.
1 participant
Summary by CodeRabbit