Use null, override, default#2473
Conversation
* TAO/docs/releasenotes/orbcore.html:
* TAO/tao/Acceptor_Impl.cpp:
* TAO/tao/Connection_Purging_Strategy.cpp:
* TAO/tao/Connection_Purging_Strategy.h:
* TAO/tao/LRU_Connection_Purging_Strategy.cpp:
* TAO/tao/LRU_Connection_Purging_Strategy.h:
* TAO/tao/Strategies/FIFO_Connection_Purging_Strategy.h:
* TAO/tao/Strategies/LFU_Connection_Purging_Strategy.h:
* TAO/tao/Strategies/NULL_Connection_Purging_Strategy.h:
* TAO/tao/Transport_Cache_Manager_T.cpp:
* TAO/tao/Transport_Cache_Manager_T.h:
* TAO/tao/Transport_Timer.h:
* TAO/tao/default_resource.cpp:
* TAO/tao/orbconf.h:
WalkthroughThis pull request modernizes C++ coding patterns across TAO by replacing integer null pointer literals (0) with nullptr, adding override specifiers to virtual function implementations, making destructors defaulted, and adding const/in-class initialization to member variables. Minor textual and formatting corrections are also included. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Rationale: While most changes are mechanical (nullptr replacement, override specifiers, formatting), the modifications to member variable initialization in LRU_Connection_Purging_Strategy (removal of explicit initialization in constructor coupled with in-class Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 2
🧹 Nitpick comments (5)
TAO/tao/Acceptor_Impl.cpp (1)
33-37: Prefer nullptr over 0 in base ctor init.Minor consistency nit: replace literal 0 with nullptr in ACE_Creation_Strategy base init.
Apply this diff:
- : ACE_Creation_Strategy<SVC_HANDLER> (0, orb_core->reactor()), + : ACE_Creation_Strategy<SVC_HANDLER> (nullptr, orb_core->reactor()),TAO/tao/Transport_Cache_Manager_T.cpp (2)
387-388: Early return on nullptr is correct; drop redundant (void)Guard is good. The subsequent call uses a void-returning method; the (void) cast is unnecessary.
- purging_strategy *st = this->purging_strategy_; - (void) st->update_item (*(entry->item ().transport ())); + purging_strategy *st = this->purging_strategy_; + st->update_item (*(entry->item ().transport ()));
98-98: Consider completing the nullptr migrationLine 98 correctly uses
nullptrfor pointer initialization. The file contains many remaining instances where0is used for pointer initialization and comparison that could be converted tonullptrfor consistency with modern C++ practices, including:
- Member initializations: lines 35, 38–39
- Pointer variable declarations: lines 125, 283, 286
- Pointer assignments: lines 232, 292, 413, 568, 631, 712
- Pointer comparisons: lines 356, 467, 517, 574
This is an optional refactoring to improve consistency across the file.
TAO/tao/Strategies/FIFO_Connection_Purging_Strategy.h (1)
44-49: override looks good; align init/dtor with LRU for consistencyRecommend default-initializing order_ and defaulting the virtual destructor (if present in impl) for parity with LRU.
class TAO_Strategies_Export TAO_FIFO_Connection_Purging_Strategy: public TAO_Connection_Purging_Strategy { public: /// The constructor TAO_FIFO_Connection_Purging_Strategy (int cache_maximum); /// Called when accessing an item from the cache void update_item (TAO_Transport& transport) override; private: /// The ordering information for each transport in the cache - unsigned long order_; + unsigned long order_ {}; };If a custom destructor exists out-of-line, consider:
- // existing dtor (if any) + ~TAO_FIFO_Connection_Purging_Strategy () override = default;TAO/tao/LRU_Connection_Purging_Strategy.h (1)
43-52: Nice modernization: defaulted dtor, override, in-class initAll three improve clarity and safety. Optionally mark the single-arg ctor explicit (see cpp comment) for uniformity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
TAO/docs/releasenotes/orbcore.html(1 hunks)TAO/tao/Acceptor_Impl.cpp(1 hunks)TAO/tao/Connection_Purging_Strategy.cpp(0 hunks)TAO/tao/Connection_Purging_Strategy.h(2 hunks)TAO/tao/LRU_Connection_Purging_Strategy.cpp(1 hunks)TAO/tao/LRU_Connection_Purging_Strategy.h(1 hunks)TAO/tao/Strategies/FIFO_Connection_Purging_Strategy.h(1 hunks)TAO/tao/Strategies/LFU_Connection_Purging_Strategy.h(1 hunks)TAO/tao/Strategies/NULL_Connection_Purging_Strategy.h(1 hunks)TAO/tao/Transport_Cache_Manager_T.cpp(4 hunks)TAO/tao/Transport_Cache_Manager_T.h(1 hunks)TAO/tao/Transport_Timer.h(1 hunks)TAO/tao/default_resource.cpp(1 hunks)TAO/tao/orbconf.h(1 hunks)
💤 Files with no reviewable changes (1)
- TAO/tao/Connection_Purging_Strategy.cpp
🧰 Additional context used
🧬 Code graph analysis (6)
TAO/tao/LRU_Connection_Purging_Strategy.h (1)
TAO/tao/LRU_Connection_Purging_Strategy.cpp (4)
TAO_LRU_Connection_Purging_Strategy(4-9)TAO_LRU_Connection_Purging_Strategy(6-7)update_item(11-15)update_item(12-12)
TAO/tao/Strategies/LFU_Connection_Purging_Strategy.h (1)
TAO/tao/LRU_Connection_Purging_Strategy.cpp (2)
update_item(11-15)update_item(12-12)
TAO/tao/Strategies/NULL_Connection_Purging_Strategy.h (3)
TAO/tao/LRU_Connection_Purging_Strategy.cpp (2)
update_item(11-15)update_item(12-12)TAO/tao/default_resource.cpp (2)
cache_maximum(1006-1010)cache_maximum(1007-1007)TAO/tao/Connection_Purging_Strategy.cpp (1)
cache_maximum(10-10)
TAO/tao/default_resource.cpp (2)
TAO/tao/LRU_Connection_Purging_Strategy.cpp (2)
TAO_LRU_Connection_Purging_Strategy(4-9)TAO_LRU_Connection_Purging_Strategy(6-7)TAO/tao/LRU_Connection_Purging_Strategy.h (1)
TAO_LRU_Connection_Purging_Strategy(36-52)
TAO/tao/Strategies/FIFO_Connection_Purging_Strategy.h (1)
TAO/tao/LRU_Connection_Purging_Strategy.cpp (2)
update_item(11-15)update_item(12-12)
TAO/tao/LRU_Connection_Purging_Strategy.cpp (1)
TAO/tao/LRU_Connection_Purging_Strategy.h (1)
TAO_LRU_Connection_Purging_Strategy(36-52)
⏰ 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: macos-14-C++
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-24.04 g++-10 wchar
- GitHub Check: ubuntu-24.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 g++-10 CodeQL
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: ubuntu-22.04 clang++-13
- GitHub Check: ubuntu-24.04 clang++-20
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-24.04 clang++-20 c++23
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: ubuntu-22.04 g++-9
🔇 Additional comments (12)
TAO/docs/releasenotes/orbcore.html (1)
147-147: Typo fix LGTM.“part of the TAO_Messaging” reads correctly now.
TAO/tao/orbconf.h (1)
154-156: Comment tweak LGTM.No functional change; clearer wording.
TAO/tao/Acceptor_Impl.cpp (1)
43-51: nullptr check LGTM.Matches the PR’s modernization goal and is semantically correct.
TAO/tao/Transport_Cache_Manager_T.h (1)
54-56: Spelling fix LGTM.Documentation clarity improved; no code impact.
TAO/tao/Transport_Timer.h (1)
43-43: explicit constructor LGTM.Prevents unintended implicit conversions.
TAO/tao/default_resource.cpp (1)
1075-1080: LRU strategy construction cleanup LGTM.Behavior unchanged; reads clearer.
TAO/tao/Connection_Purging_Strategy.h (2)
45-45: Defaulted virtual destructor verified—no ODR concerns.Verification confirms no out-of-line destructor definition exists in implementation files (Connection_Purging_Strategy.inl or .cpp). The defaulted inline declaration at line 45 is the only definition, so the change is safe from an ODR perspective.
55-56: Verified: cache_maximum_ const is safe; no reassignments found.The member
cache_maximum_is already declared asconstin the header. Verification found only one assignment tocache_maximum_at line 321 indefault_resource.cpp, which occurs during constructor initialization of a different class (TAO_Default_Resource_Factory). No writes tocache_maximum_exist withinConnection_Purging_Strategyitself post-construction. The const qualifier is correctly applied and safe.TAO/tao/Strategies/LFU_Connection_Purging_Strategy.h (1)
43-43: override addition is correctClearer intent; no behavioral change.
TAO/tao/Transport_Cache_Manager_T.cpp (2)
79-83: Post-delete nullptr assignment is harmlessSets deleted pointers to nullptr; safe though not strictly necessary in a destructor. Consistent with the file’s nullptr shift.
435-435: Minor style tidy is fineLocal const bool improves readability; no behavior change.
TAO/tao/Strategies/NULL_Connection_Purging_Strategy.h (1)
41-44: LGTM! Good modernization with theoverridespecifier.Adding the
overridekeyword to bothupdate_itemandcache_maximumis an excellent C++11 modernization that provides compile-time verification that these methods correctly override base class virtuals. This enhances type safety and makes the code more maintainable.
| TAO_LRU_Connection_Purging_Strategy::TAO_LRU_Connection_Purging_Strategy (int cache_maximum) | ||
| : TAO_Connection_Purging_Strategy (cache_maximum) | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
Ctor cleanup aligned with in-class init; LGTM
Relying on header’s order_ {} is fine. Consider making the ctor explicit to avoid accidental implicit conversions from int.
Diff (header):
- TAO_LRU_Connection_Purging_Strategy (int cache_maximum);
+ explicit TAO_LRU_Connection_Purging_Strategy (int cache_maximum);Run to confirm a key out-of-line virtual exists (vtable anchor):
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*void\s+TAO_LRU_Connection_Purging_Strategy::update_item\s*\(' tao -g '!**/test/**'Length of output: 43
I need to explore the repository structure since the previous path didn't work. Let me search for the relevant files:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the LRU_Connection_Purging_Strategy files
fd 'LRU_Connection_Purging_Strategy' --type f
Length of output: 85
🏁 Script executed:
#!/bin/bash
# List repository structure at root
ls -la | head -20Length of output: 584
🏁 Script executed:
#!/bin/bash
# Search for update_item method in the codebase
rg -nP 'update_item' --type cpp --type h -C 2Length of output: 5005
Now let me examine the header file to verify the in-class initialization and check the current state of the constructor:
---
🏁 Script executed:
```shell
#!/bin/bash
# View the header file to check order_ in-class init and explicit keyword
cat TAO/tao/LRU_Connection_Purging_Strategy.h
Length of output: 1502
🏁 Script executed:
#!/bin/bash
# View the constructor implementation
cat TAO/tao/LRU_Connection_Purging_Strategy.cppLength of output: 424
Add explicit keyword to constructor to prevent implicit conversions
The verification confirms all core aspects are sound:
- The vtable anchor
update_item()exists out-of-line (line 12 of .cpp) - Header has
order_ {}in-class initialization (line 49) - Constructor implementation (lines 6-8) correctly relies on the header's in-class init
However, the suggested explicit keyword has not yet been applied. Without it, the constructor allows implicit conversion from int to TAO_LRU_Connection_Purging_Strategy, which should be prevented.
Diff (header, line 39):
- TAO_LRU_Connection_Purging_Strategy (int cache_maximum);
+ explicit TAO_LRU_Connection_Purging_Strategy (int cache_maximum);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
TAO/tao/LRU_Connection_Purging_Strategy.cpp lines 6-8: the constructor allows
implicit conversion from int to TAO_LRU_Connection_Purging_Strategy; to fix,
modify the constructor declaration in the header at line 39 to be explicit (i.e.
add the explicit keyword before the constructor declaration) so implicit
conversions are prevented while leaving the .cpp implementation unchanged.
| /// TAO_Transport | ||
| virtual int handle_timeout (const ACE_Time_Value ¤t_time, | ||
| const void *act); | ||
| int handle_timeout (const ACE_Time_Value ¤t_time, const void *act) override; |
There was a problem hiding this comment.
🧩 Analysis chain
Keep default parameter to avoid source breaks.
ACE_Event_Handler::handle_timeout typically provides a default for the second arg. Dropping the default here may break code that calls handle_timeout(current_time) via TAO_Transport_Timer static type. Keep override but retain the default.
Apply this diff:
- int handle_timeout (const ACE_Time_Value ¤t_time, const void *act) override;
+ int handle_timeout (const ACE_Time_Value ¤t_time,
+ const void *act = nullptr) override;Please confirm the base signature in your ACE version includes a default for act and that no calls in TAO rely on the derived default. To check:
🏁 Script executed:
#!/bin/bash
# Locate ACE base declaration (adjust path if needed)
fd -a Event_Handler.h | xargs rg -n "handle_timeout\s*\(.*const\s+void\s*\*\s*\w*\s*=\s*0"
# Find potential 1-arg calls on TAO_Transport_Timer
rg -nP --type=cpp -C2 '\bTAO_Transport_Timer\b.*\bhandle_timeout\s*\(\s*[^,()]+\s*\)\s*;' -g '!**/generated/**'Length of output: 0
I'll verify this in multiple ways since the initial script didn't find results (likely because ACE is external). Let me inspect the Transport_Timer.h file directly and search for handle_timeout calls.
Good news—the ACE base signature does include a default parameter: const void *act = 0. Now let me verify the Transport_Timer.h file and search for any single-argument calls to handle_timeout in the TAO codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and inspect Transport_Timer.h
fd Transport_Timer.h -x cat {}
# Search for all handle_timeout calls in TAO
echo "=== Searching for handle_timeout calls ==="
rg -nP "handle_timeout\s*\(" --type=cpp -g '!**/generated/**' -A1
Length of output: 73568
Now let me verify if there are any direct calls to TAO_Transport_Timer::handle_timeout with a single argument in the codebase to be thorough:Perfect. The web search confirms that ACE_Event_Handler::handle_timeout has const void *act = 0 as the default parameter. The review comment is accurate and the recommendation is sound.
Add default parameter to handle_timeout to maintain compatibility with base class contract.
The ACE_Event_Handler base class declares handle_timeout with a default parameter const void *act = 0. Removing this default in the derived TAO_Transport_Timer class breaks source compatibility—code that calls handle_timeout(current_time) via the derived type will fail to compile. This is inconsistent with other connection handler classes in TAO that retain the default.
Apply this diff:
- int handle_timeout (const ACE_Time_Value ¤t_time, const void *act) override;
+ int handle_timeout (const ACE_Time_Value ¤t_time,
+ const void *act = nullptr) override;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int handle_timeout (const ACE_Time_Value ¤t_time, const void *act) override; | |
| int handle_timeout (const ACE_Time_Value ¤t_time, | |
| const void *act = nullptr) override; |
🤖 Prompt for AI Agents
In TAO/tao/Transport_Timer.h around line 47, the derived declaration of int
handle_timeout(const ACE_Time_Value ¤t_time, const void *act) override is
missing the default value for the second parameter; update the declaration to
include the default argument (const void *act = 0) so it matches the
ACE_Event_Handler base signature and preserves source compatibility, keeping the
override specifier intact.
1 participant
Summary by CodeRabbit
Documentation
Style