chore: Fix a few sonar issues in Cuboid portal shell #4113
Conversation
WalkthroughChanged, much has been. New abstract and concrete classes for cuboid and cylinder portal shells introduced, they have been, along with enhanced methods for setting and retrieving portals. Header files, renamed methods, and updated dependency inclusions now guide the geometry subsystem. Unit tests and documentation, they have been updated as well. Error handling, improved it is, with assertions replaced by exception throws. Overall, structure and clarity, improved they have. Changes
Sequence Diagram(s)sequenceDiagram
participant TV as TrackingVolume
participant SCP as SingleCuboidPortalShell
participant CSP as CuboidStackPortalShell
participant CP as CuboidPortalShell (Interface)
TV->>SCP: Request portal(face)
SCP-->>TV: Return portal instance
TV->>CSP: Pass volume for multi-shell filling
CSP->>SCP: Delegate portal management
SCP->>CP: Set/retrieve portal as needed
sequenceDiagram
participant TV as TrackingVolume
participant SCP as SingleCylinderPortalShell
participant CST as CylinderStackPortalShell
participant CP as CylinderPortalShell (Interface)
TV->>SCP: Create and assign cylinder portal
SCP-->>TV: Return portal via portal(face)
TV->>CST: Aggregate multiple cylinder shells
CST->>CP: Manage portal operations consistently
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
github-actions
bot
added
Component - Core
![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 (4)
Core/src/Geometry/CuboidVolumeBounds.cpp (1)
172-185: Check consistent usage of AxisDirection, hmmm.Well-structured this function is, yes. Factor out repeated "Invalid axis direction" message, you could, to keep the code more uniform and avoid duplication, hmmm.
Core/include/Acts/Geometry/CuboidPortalShell.hpp (2)
63-98: Careful with raw pointers, you should be.Yes, storing
TrackingVolume* m_volumeis convenient. If volume’s lifetime guaranteed beyond this shell it is not, risk of dangling pointer we have. Consider astd::shared_ptr<TrackingVolume>or references if lifetime is well-defined, you might.
100-153: A composite shell, you have built.Stacking direction checked you must, yes? Validate that
shellsvector is not empty and that all shells share consistent geometry parameters for the stack, you should. Potential bugs avoided, they will be.Need a helper function for the geometry consistency check, do you?
Core/include/Acts/Geometry/CylinderPortalShell.hpp (1)
63-95: A single cylinder shell, hrrm. Correct design, yes.Yet watch for concurrency issues you must, if volumes updated or replaced concurrently. Data races, cause trouble they can. Safeguards, you might place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Core/include/Acts/Geometry/CuboidPortalShell.hpp(1 hunks)Core/include/Acts/Geometry/CuboidVolumeBounds.hpp(3 hunks)Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/CylinderPortalShell.hpp(1 hunks)Core/include/Acts/Geometry/MaterialDesignatorBlueprintNode.hpp(1 hunks)Core/include/Acts/Geometry/PortalShell.hpp(1 hunks)Core/src/Geometry/Blueprint.cpp(1 hunks)Core/src/Geometry/CMakeLists.txt(1 hunks)Core/src/Geometry/CuboidPortalShell.cpp(1 hunks)Core/src/Geometry/CuboidVolumeBounds.cpp(2 hunks)Core/src/Geometry/CuboidVolumeStack.cpp(12 hunks)Core/src/Geometry/CylinderContainerBlueprintNode.cpp(1 hunks)Core/src/Geometry/CylinderPortalShell.cpp(1 hunks)Core/src/Geometry/StaticBlueprintNode.cpp(1 hunks)Core/src/Geometry/TrivialPortalLink.cpp(2 hunks)Tests/UnitTests/Core/Geometry/CMakeLists.txt(1 hunks)Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp(1 hunks)Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp(5 hunks)Tests/UnitTests/Core/Geometry/CylinderPortalShellTests.cpp(1 hunks)docs/core/geometry/concepts.md(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- Core/src/Geometry/CylinderPortalShell.cpp
- Core/src/Geometry/StaticBlueprintNode.cpp
- Tests/UnitTests/Core/Geometry/CuboidVolumeStackTests.cpp
- Core/src/Geometry/Blueprint.cpp
- Core/src/Geometry/CylinderContainerBlueprintNode.cpp
- Core/src/Geometry/CuboidVolumeStack.cpp
🧰 Additional context used
🧠 Learnings (1)
Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp (1)
Learnt from: paulgessinger
PR: acts-project/acts#3869
File: Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp:28-29
Timestamp: 2024-12-05T20:21:24.253Z
Learning: In `CylinderContainerBlueprintNode`, validation that child volumes are cylinders is enforced by `CylinderVolumeStack`.
🪛 Cppcheck (2.10-2)
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp
[error] 194-194: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (24)
Core/src/Geometry/CMakeLists.txt (1)
46-47: Updated source files to use specialized portal shells, hmm.Replaced the generic
PortalShell.cppwith specialized implementationsCylinderPortalShell.cppandCuboidPortalShell.cpp. Good refactoring this is, yes! Better separation of concerns, it provides.docs/core/geometry/concepts.md (1)
57-64: Documentation for new cuboid portal shell classes, added it has been.Properly documented the new classes are:
Acts::CuboidPortalShellActs::SingleCuboidPortalShellActs::CuboidStackPortalShellConsistent with existing documentation style, this is. Clear understanding for users, it provides.
Tests/UnitTests/Core/Geometry/CylinderPortalShellTests.cpp (1)
17-17: Updated include to use the specialized header, wise this is.Replaced the generic
PortalShell.hppinclude with the specificCylinderPortalShell.hpp. More precise dependencies, this creates. Aligns with the refactoring of portal shell classes, it does.Tests/UnitTests/Core/Geometry/CMakeLists.txt (1)
38-39: Adding new unittests, wise move it is
Broader coverage for CylinderPortalShell and CuboidPortalShell, you achieve. Verify edge cases, you must.Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp (1)
12-12: Include CylinderPortalShell, hmm?
Consistent with the new geometry approach this is. Confirm other references are updated, you should.Core/src/Geometry/TrivialPortalLink.cpp (2)
13-13: ThrowAssert you include, improved error handling it yields
No longer rely solely on debug builds, you do. Flexible and safer this method is.
36-38: To throw you choose, if surface check fails
Silent misbehavior, you avoid. Clear and safe, this approach remains.Core/include/Acts/Geometry/PortalShell.hpp (1)
11-12: Add cstddef and string includes, done you have
Needed they are, suspect I do. Harmless to keep, unless unused they become.Core/src/Geometry/CuboidVolumeBounds.cpp (1)
187-207: Simple and effective, it appears.Implementation aligned well with the intended face logic, yes. Merged neatly, your return of negative/positive faces and side faces is.
Core/include/Acts/Geometry/CuboidVolumeBounds.hpp (3)
12-12: Needed include, this is.Synchronized with new face definition, it aligns.
62-72: Introduced enum Face, strong clarity it brings.Mapped to boundary surfaces, well it is.
172-182: Method declarations consistent, they are.Match the definitions in the source file, they do.
Tests/UnitTests/Core/Geometry/CuboidPortalShellTests.cpp (5)
55-137: ConstructionFromVolume tests thorough, yes.Validates creation with correct and incorrect bounds, it does. Coverage strong, seeing that
std::invalid_argumentis thrown, good.
139-192: Portal assignment check, well done.Verifies replacement of portals and ensures unaffected faces remain correct, yes.
194-353: Parametric testing, make sense it does.Covers multiple directions by data-driven approach, good coverage you have.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 194-194: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
354-743: Nested stacks tested in detail, hmm.Ensuring combined volumes align, strong verification it is. Good approach, you have.
744-781: Filling and registering volumes, tested thoroughly.Checks internal portal linkage, nice coverage.
Core/src/Geometry/CuboidPortalShell.cpp (5)
30-39: Filling logic, yes.Through each face we fill, ensuring volume’s portals are updated, correct.
41-66: Constructor validity check, done well.Throws for non-cuboid bounds, consistent with design.
67-114: Portal getters and setters, hmmm.Clean and readable, they are. Corner cases appear handled.
116-235: Stacked shell logic, quite robust.Sorting by coordinate and merging portals in base/back faces, well done. Possibly watch for expansions if future axes appear.
236-311: Validation, transformations, labeling, complete they are.Code cohesive and consistent with the design, it is.
Core/include/Acts/Geometry/CuboidPortalShell.hpp (1)
27-50: Design is strong, hmmm. Extend it well, you do.A sound base class for cuboid portal shells, yes. Still, verify you must that all face accesses (e.g.,
portal(Face face)) handle unexpected faces or out-of-bounds usage gracefully, hmmm.Core/include/Acts/Geometry/CylinderPortalShell.hpp (1)
24-48: A flexible base, hmmm.Yes,
CylinderPortalShellwise it seems. Ensure you do that uninitialized faces cause no runtime confusion. Enforce constraints for faces and radial bounds, best practice it is.
github-actions
bot
removed
the
Component - Documentation
|
Blocked by:
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit