fix: Remove final on template derived classes
#3923
Conversation
This seems to cause issued when using `dynamic_cast` on AppleClang 16 and llvm 17 on macOS 15 (at least). Reproducer: https://gist.github.com/paulgessinger/2c1d2abdeb322072c507878ab5833728
paulgessinger
changed the title
final on teplate derived classesfinal on template derived classes
WalkthroughChanges made, significant they are. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Outside diff range and nitpick comments (5)
Core/include/Acts/Utilities/GridAccessHelpers.hpp (2)
145-145: Good, this change is! Documentation, enhance we should.Remove
finalspecifier, we must, fordynamic_castto function properly. Yet, document the inheritance capabilities in class documentation, we should. Help future Padawans understand the design intentions, it will.Consider adding this documentation above the class:
/// @brief A global subspace transformation class that can be inherited from /// to customize the transformation behavior
Line range hint
113-182: Architectural wisdom, share I must.Template inheritance, a powerful tool it is, but use it wisely we must. Consider these aspects, you should:
- Template instantiation overhead, watch for it
- Binary size impact, measure you must
- Consistent inheritance patterns across codebase, maintain we should
Core/include/Acts/Utilities/Axis.hpp (1)
Line range hint
100-420: Virtual destructor, consider adding you must.When inheritance allowed becomes, virtual destructor crucial it is. Through base pointer when derived object deleted is, undefined behavior without virtual destructor occurs.
Add virtual destructor to IAxis base class or protected non-virtual destructor if public inheritance not intended:
class IAxis { + virtual ~IAxis() = default; // ... existing members ... };Core/include/Acts/Geometry/GridPortalLink.hpp (2)
Line range hint
412-445: More descriptive error messages, suggest I do!Improve error handling clarity, we must. When invalid surface type or direction encountered is, more context in error messages, helpful would be.
Consider this improvement:
- throw std::invalid_argument{"Invalid binning direction"}; + throw std::invalid_argument{ + "Invalid binning direction for CylinderSurface. Expected binRPhi or binZ, got " + + std::to_string(static_cast<int>(direction))};Similar improvements for other error messages, apply you should.
Line range hint
551-558: Safer bounds checking, implement we must!Assert statement in production code, dangerous it can be. Replace with proper error handling, we should.
Consider this improvement:
- assert(surface().insideBounds(position, BoundaryTolerance::None())); + if (!surface().insideBounds(position, BoundaryTolerance::None())) { + return Result<const TrackingVolume*>::failure( + std::runtime_error("Position not within surface bounds")); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Core/include/Acts/Geometry/GridPortalLink.hpp(1 hunks)Core/include/Acts/Utilities/Axis.hpp(2 hunks)Core/include/Acts/Utilities/GridAccessHelpers.hpp(3 hunks)
🔇 Additional comments (6)
Core/include/Acts/Utilities/GridAccessHelpers.hpp (3)
113-113: Wise decision, removing final is! Hmmmm.
Align with the PR objectives, this change does. Allow dynamic_cast to work correctly on AppleClang 16 and LLVM 17, it will. Inheritance hierarchy, preserved it is.
182-182: Approve this change, I do! Yet verify consistency, we must.
Remove final specifier, wise it is. With the Force of dynamic_cast, align it does.
Line range hint 224-224: Consistency in the Force, sense I do not. Hmmmm.
Still marked as final, the BoundCylinderToZPhi class is. Verify we must, if same treatment it needs.
✅ Verification successful
Remove 'final' keyword, we must not!
Correct the marking as 'final' is, young padawan! Through the Force, I see that BoundCylinderToZPhi used only through dynamic casting and factory methods it is. No inheritance from it exists, and proper it is to prevent it. Base class IBoundToGridLocal serves as the interface, it does, while BoundCylinderToZPhi remains a concrete leaf class, hmmmm.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of BoundCylinderToZPhi with dynamic_cast
rg "dynamic_cast.*BoundCylinderToZPhi"
# Check if BoundCylinderToZPhi is used as a base class
ast-grep --pattern 'class $_ : public BoundCylinderToZPhi'
Length of output: 341
Script:
#!/bin/bash
# Search for the BoundCylinderToZPhi class definition
ast-grep --pattern 'class BoundCylinderToZPhi {
$$$
}'
# Check if there are any virtual methods in BoundCylinderToZPhi
rg -A 5 "virtual.*BoundCylinderToZPhi"
# Look for inheritance from BoundCylinderToZPhi
rg "BoundCylinderToZPhi" -A 2 -B 2
Length of output: 4102
Script:
#!/bin/bash
# Search for the base class IBoundToGridLocal definition and usage
ast-grep --pattern 'class IBoundToGridLocal {
$$$
}'
# Check if there are any other classes inheriting from IBoundToGridLocal
rg "public IBoundToGridLocal" -B 2 -A 2
Length of output: 862
Core/include/Acts/Utilities/Axis.hpp (2)
100-100: Approve removal of final specifier from Equidistant axis class, I do.
Wise decision this is, allowing inheritance from template class and fixing dynamic_cast issues on AppleClang it does. Maintain functionality while extending flexibility, it does.
420-420: Approve removal of final specifier from Variable axis class, I do.
Similar to Equidistant axis, wise change this is. Dynamic casting issues on macOS platform, resolve it does.
Core/include/Acts/Geometry/GridPortalLink.hpp (1)
399-399: Approve the removal of final specifier, I do!
Wise decision this is, allowing proper inheritance and fixing dynamic_cast issues on AppleClang 16 and LLVM 17, it does. Safe and necessary change, this appears to be.
|
- I missed a `final` in one location (see #3923) - Adding an exception to `GridAccessJsonConverter::toJson`. This was previously silently returning an empty json object if none of the downcasts worked. Since the downcast chain is not exhaustive, I think it's better to throw if there's an unhandled case.
This seems to cause issues when using
dynamic_caston AppleClang 16 and llvm 18 on macOS 15 (at least).Reproducer:
https://gist.github.com/paulgessinger/2c1d2abdeb322072c507878ab5833728
Summary by CodeRabbit
New Features
GridPortalLinkT,Axis,Affine3Transformed,GlobalSubspace, andLocalSubspaceclasses by allowing subclassing.GridPortalLinkTto improve error handling and functionality.Bug Fixes
resolveVolumemethod to ensure positions are within valid bounds.Documentation