feat(apps): build individual subsystems from build_geometry - #52
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds registry-based subsystem assembly, self-registering factories, selectable full or subsystem geometry builds, linker retention for registration initializers, and updated README guidance. ChangesSubsystem Registry Refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
🧹 Nitpick comments (1)
include/SHiPGeometry/SubsystemRegistry.h (1)
54-57: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFail fast on duplicate subsystem names.
emplace()silently ignores a second registration with the samedesc.name, but still returnstrue; a duplicated descriptor would make one subsystem disappear from--list/assembly without any diagnostic.Proposed fix
+#include <stdexcept> + inline bool registerSubsystem(const SubsystemDescriptor& desc, std::function<GeoVPhysVol*(SHiPMaterials&)> build) { - registry().emplace(desc.name, SubsystemInfo{desc, std::move(build)}); + auto [_, inserted] = + registry().emplace(desc.name, SubsystemInfo{desc, std::move(build)}); + if (!inserted) { + throw std::runtime_error("Duplicate subsystem registration: '" + + std::string(desc.name) + "'"); + } return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/SHiPGeometry/SubsystemRegistry.h` around lines 54 - 57, The registerSubsystem function currently returns success even when a duplicate desc.name is registered, because registry().emplace() silently ignores the second entry. Update registerSubsystem in SubsystemRegistry.h to detect an existing name before or after emplace, and fail fast by rejecting the duplicate instead of returning true. Use the registerSubsystem and registry() symbols to locate the check, and make sure duplicate subsystem descriptors cannot be added without a clear diagnostic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/build_geometry.cpp`:
- Around line 83-85: The output-file cleanup in build_geometry.cpp uses the
throwing std::filesystem::remove overload after a redundant exists() check,
which can crash on permission or lock failures. Update the logic around the
outputFile deletion to call the non-throwing remove variant and handle its
result (and any error_code) in build_geometry so missing files are treated as a
no-op while real filesystem errors are reported cleanly.
- Around line 55-76: The default full-build path in build_geometry.cpp lacks the
same exception handling used for SHiPGeometry::buildSubsystem, so
builder.build() can terminate on a thrown std::runtime_error. Wrap the
names.empty() branch in the same try/catch pattern used for the single-subsystem
path, catch std::exception, print a clear error plus available subsystems if
helpful, and return 1 so buildGeometry fails gracefully instead of aborting.
In `@include/SHiPGeometry/SubsystemRegistry.h`:
- Around line 8-11: The header is not self-contained because
`SubsystemRegistry.h` uses `std::move` without including the declaration it
comes from. Add the missing `<utility>` include alongside the existing includes
in `SubsystemRegistry.h` so any translation unit using `SubsystemRegistry`
compiles independently.
In `@README.md`:
- Line 170: The fenced code block in the README is missing a language
identifier, which triggers markdownlint MD040. Update that markdown fence to
include a language specifier, using the fenced block in the README around the
geometry/ snippet as the target so the block is properly annotated.
In `@src/SHiPGeometry.cpp`:
- Around line 50-59: The world assembly path currently assumes
entry.second.build(materials) always returns a valid geometry, but a factory may
return null and lead to a bad cast or later GeoModel failure. In the world
selection logic in SHiPGeometry::buildSubsystem, check the build result before
dynamic_cast<GeoPhysVol*> and throw a clear error if it is null; apply the same
validation anywhere build results are passed into place() so null subsystem
geometries are rejected at the assembly boundary instead of propagating deeper.
In `@subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp`:
- Line 5: The UpstreamTagger registration path is losing the SHiPUBTManager
pointer, so build() never receives a valid manager and setContainerVolume() is
skipped. Update the REGISTER_SUBSYSTEM wiring in UpstreamTaggerFactory so the
registry callback preserves and forwards the SHiPUBTManager* (or otherwise
retrieves it) into build(), and ensure both buildSubsystem() and
assembleGeometry() use the same manager-aware path.
---
Nitpick comments:
In `@include/SHiPGeometry/SubsystemRegistry.h`:
- Around line 54-57: The registerSubsystem function currently returns success
even when a duplicate desc.name is registered, because registry().emplace()
silently ignores the second entry. Update registerSubsystem in
SubsystemRegistry.h to detect an existing name before or after emplace, and fail
fast by rejecting the duplicate instead of returning true. Use the
registerSubsystem and registry() symbols to locate the check, and make sure
duplicate subsystem descriptors cannot be added without a clear diagnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f28d3d8-52fb-4061-8ca5-100ab3561b6c
📒 Files selected for processing (25)
README.mdapps/build_geometry.cppinclude/SHiPGeometry/SubsystemRegistry.hsrc/CMakeLists.txtsrc/SHiPGeometry.cppsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Cavern/include/Cavern/CavernFactory.hsubsystems/Cavern/src/CavernFactory.cppsubsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/Magnet/include/Magnet/MagnetFactory.hsubsystems/Magnet/src/MagnetFactory.cppsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.hsubsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cppsubsystems/Target/include/Target/TargetFactory.hsubsystems/Target/src/TargetFactory.cppsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.hsubsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
| if (std::filesystem::exists(outputFile)) { | ||
| std::filesystem::remove(outputFile); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
std::filesystem::remove can throw uncaught on permission/lock errors.
The throwing overload is used; a locked or permission-denied output file would crash the tool instead of producing a clean error. The preceding exists() check is also redundant since remove() returns false harmlessly when the path doesn't exist.
🛡️ Proposed fix
- if (std::filesystem::exists(outputFile)) {
- std::filesystem::remove(outputFile);
- }
+ std::error_code ec;
+ std::filesystem::remove(outputFile, ec);
+ if (ec) {
+ std::cerr << "Warning: could not remove existing " << outputFile << ": " << ec.message()
+ << std::endl;
+ }📝 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.
| if (std::filesystem::exists(outputFile)) { | |
| std::filesystem::remove(outputFile); | |
| } | |
| std::error_code ec; | |
| std::filesystem::remove(outputFile, ec); | |
| if (ec) { | |
| std::cerr << "Warning: could not remove existing " << outputFile << ": " << ec.message() | |
| << std::endl; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/build_geometry.cpp` around lines 83 - 85, The output-file cleanup in
build_geometry.cpp uses the throwing std::filesystem::remove overload after a
redundant exists() check, which can crash on permission or lock failures. Update
the logic around the outputFile deletion to call the non-throwing remove variant
and handle its result (and any error_code) in build_geometry so missing files
are treated as a no-op while real filesystem errors are reported cleanly.
|
|
||
| ## Structure | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced code block.
markdownlint flags this block (MD040) since it has no language specified.
📝 Proposed fix
-```
+```text
geometry/🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 170-170: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 170, The fenced code block in the README is missing a
language identifier, which triggers markdownlint MD040. Update that markdown
fence to include a language specifier, using the fenced block in the README
around the geometry/ snippet as the target so the block is properly annotated.
Source: Linters/SAST tools
There was a problem hiding this comment.
I think this is not actually resolved.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- README structure section ---'
nl -ba README.md | sed -n '160,195p'
printf '%s\n' '--- Markdown lint configuration files ---'
fd -HI '(^|/)(\.markdownlint.*|markdownlint.*)$' . -t f -x sh -c 'printf "\n--- %s ---\n" "$1"; cat "$1"' sh {} \; || true
printf '%s\n' '--- All unlabeled Markdown fences in README.md ---'
awk '
/^```[[:space:]]*$/ { print "Line " NR ": " $0 }
' README.mdLength of output: 816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- README structure section (numbered) ---'
awk 'NR >= 160 && NR <= 195 { printf "%d:%s\n", NR, $0 }' README.md
printf '%s\n' '--- Markdown lint configuration files ---'
find . -type f \( -name '.markdownlint*' -o -name 'markdownlint*' \) \
-not -path './.git/*' \
-print \
-exec sh -c 'printf "\n--- %s ---\n" "$1"; cat "$1"' sh {} \;
printf '%s\n' '--- Unlabeled Markdown fences in README.md ---'
awk '/^```[[:space:]]*$/ { printf "Line %d: %s\n", NR, $0 }' README.mdLength of output: 2577
@olantwin, you’re right — it is not resolved. README.md still has an unlabeled opening fence at Line 180 for the geometry/ structure tree, so it still triggers MD040.
The minimal fix remains:
```text
geometry/
...
I’ll keep this finding unresolved.
| // Copyright (C) CERN for the benefit of the SHiP Collaboration | ||
|
|
||
| #include "UpstreamTagger/UpstreamTaggerFactory.h" | ||
| #include "SHiPGeometry/SubsystemRegistry.h" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how SHiPUBTManager is wired today, and whether anything besides this
# build() overload sets it, to confirm the registry path truly loses it.
rg -n "SHiPUBTManager" --type=cpp -C3
rg -n "buildSubsystem|assembleGeometry" --type=cpp -C3Repository: ShipSoft/Geometry
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the factory/registry wiring.
git ls-files | rg 'UpstreamTaggerFactory\.cpp|UpstreamTagger.*\.cpp|SHiPGeometry/SubsystemRegistry\.h|SubsystemRegistry\.h|REGISTER_SUBSYSTEM'
echo '--- UpstreamTaggerFactory.cpp ---'
cat -n subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp | sed -n '1,220p'
echo '--- SubsystemRegistry.h (if present) ---'
for f in $(git ls-files | rg 'SubsystemRegistry\.h$'); do
echo "### $f"
cat -n "$f" | sed -n '1,260p'
done
echo '--- any REGISTER_SUBSYSTEM uses ---'
rg -n "REGISTER_SUBSYSTEM|build\(SHiPMaterials|SHiPUBTManager|setContainerVolume|containerPhys|GeoFullPhysVol" subsystems -C 3Repository: ShipSoft/Geometry
Length of output: 45346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect registry consumers to see whether they ever pass a manager or provide
# a post-build hook.
for f in $(git ls-files | rg 'SubsystemRegistry\.(cpp|cc|cxx|h)$|.*\.cpp$|.*\.h$'); do
if rg -n "buildSubsystem\(|assembleGeometry\(|registerSubsystem\(|setContainerVolume\(" "$f" >/dev/null 2>&1; then
echo "### $f"
rg -n "buildSubsystem\(|assembleGeometry\(|registerSubsystem\(|setContainerVolume\(" "$f" -C 4
fi
doneRepository: ShipSoft/Geometry
Length of output: 3955
REGISTER_SUBSYSTEM drops the SHiPUBTManager*, so the UpstreamTagger tree-top never gets registered through the registry path. The registry callback only forwards SHiPMaterials&, so build() always sees manager == nullptr and setContainerVolume() is skipped in both buildSubsystem() and assembleGeometry().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp` at line 5, The
UpstreamTagger registration path is losing the SHiPUBTManager pointer, so
build() never receives a valid manager and setContainerVolume() is skipped.
Update the REGISTER_SUBSYSTEM wiring in UpstreamTaggerFactory so the registry
callback preserves and forwards the SHiPUBTManager* (or otherwise retrieves it)
into build(), and ensure both buildSubsystem() and assembleGeometry() use the
same manager-aware path.
|
This PR is in preparation for another PR which will include visualisation, both for all subsystems and for individual subsystems. |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/SHiPGeometry.cpp (1)
49-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnvalidated factory build results still propagate into cast/placement.
The world lookup at Line 53 casts the factory's return value without checking for
nullptr/cast failure, and the placement loop at Line 79 passesinfo->build(materials)straight intoplace()without a null check. A misbehaving or mis-descriptored factory (e.g.isWorld=trueon a non-GeoPhysVol, or a build path that legitimately returnsnullptr) will either silently produce a null world or crash deep inside GeoModel with an unclear error.Proposed fix
GeoPhysVol* world = nullptr; for (auto& entry : reg) { if (entry.second.desc.isWorld) { - world = dynamic_cast<GeoPhysVol*>(entry.second.build(materials)); + auto* builtWorld = entry.second.build(materials); + world = dynamic_cast<GeoPhysVol*>(builtWorld); + if (!world) { + throw std::runtime_error("World subsystem '" + entry.first + + "' did not return a GeoPhysVol"); + } break; } } @@ for (const SubsystemInfo* info : placed) { - place(world, info->build(materials), info->desc); + auto* volume = info->build(materials); + if (!volume) { + throw std::runtime_error("Subsystem '" + std::string(info->desc.name) + + "' returned null geometry"); + } + place(world, volume, info->desc); }Also applies to: 78-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SHiPGeometry.cpp` around lines 49 - 59, The world selection in SHiPGeometry currently trusts factory build results without validation, so a bad isWorld subsystem or a build that returns null can propagate into the GeoPhysVol cast and later placement. In the world lookup logic, check the result of entry.second.build(materials) before dynamic_cast and verify the cast succeeded before assigning world; if either fails, throw a clear runtime error mentioning the subsystem. In the placement loop, validate info->build(materials) before calling place() and skip/throw on null so GeoModel never receives an invalid object from SHiPGeometry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 162-164: The CMake guidance in README.md is misleading because
src/CMakeLists.txt does not use a --whole-archive block for subsystem libraries;
update the instructions to reference the actual --no-as-needed / --as-needed
linker block and the existing inline comment about whole-archive being a no-op.
Make sure the step tells contributors to add the new subsystem target to that
real link section so REGISTER_SUBSYSTEM initialization is preserved.
---
Duplicate comments:
In `@src/SHiPGeometry.cpp`:
- Around line 49-59: The world selection in SHiPGeometry currently trusts
factory build results without validation, so a bad isWorld subsystem or a build
that returns null can propagate into the GeoPhysVol cast and later placement. In
the world lookup logic, check the result of entry.second.build(materials) before
dynamic_cast and verify the cast succeeded before assigning world; if either
fails, throw a clear runtime error mentioning the subsystem. In the placement
loop, validate info->build(materials) before calling place() and skip/throw on
null so GeoModel never receives an invalid object from SHiPGeometry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3f6c31d-2095-4c0a-a342-3d9cb4907ffe
📒 Files selected for processing (25)
README.mdapps/build_geometry.cppinclude/SHiPGeometry/SubsystemRegistry.hsrc/CMakeLists.txtsrc/SHiPGeometry.cppsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Cavern/include/Cavern/CavernFactory.hsubsystems/Cavern/src/CavernFactory.cppsubsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/Magnet/include/Magnet/MagnetFactory.hsubsystems/Magnet/src/MagnetFactory.cppsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.hsubsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cppsubsystems/Target/include/Target/TargetFactory.hsubsystems/Target/src/TargetFactory.cppsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.hsubsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
| 4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`, and | ||
| add the new library to the `--whole-archive` block in `src/CMakeLists.txt` | ||
| so its self-registration initializer is not dropped by the linker |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
"--whole-archive block" doesn't match the actual CMake mechanism.
src/CMakeLists.txt links subsystem targets using -Wl,--no-as-needed ... -Wl,--as-needed, with an inline comment explicitly noting whole-archive is a no-op for these shared libraries. A contributor following this step would search for a nonexistent --whole-archive block instead of the actual --no-as-needed block, risking a subsystem being left unlinked (its REGISTER_SUBSYSTEM initializer silently dropped).
📝 Proposed fix
-4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`, and
- add the new library to the `--whole-archive` block in `src/CMakeLists.txt`
- so its self-registration initializer is not dropped by the linker
+4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`, and
+ add the new library to the `--no-as-needed` linker block in
+ `src/CMakeLists.txt` so its self-registration initializer is not dropped📝 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.
| 4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`, and | |
| add the new library to the `--whole-archive` block in `src/CMakeLists.txt` | |
| so its self-registration initializer is not dropped by the linker | |
| 4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`, and | |
| add the new library to the `--no-as-needed` linker block in | |
| `src/CMakeLists.txt` so its self-registration initializer is not dropped |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 162 - 164, The CMake guidance in README.md is
misleading because src/CMakeLists.txt does not use a --whole-archive block for
subsystem libraries; update the instructions to reference the actual
--no-as-needed / --as-needed linker block and the existing inline comment about
whole-archive being a no-op. Make sure the step tells contributors to add the
new subsystem target to that real link section so REGISTER_SUBSYSTEM
initialization is preserved.
There was a problem hiding this comment.
This issue seems to still be present?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- README CMake guidance ---'
sed -n '155,172p' README.md
printf '%s\n' '--- src/CMakeLists.txt linker-related lines ---'
rg -n -C 8 -- '--whole-archive|--no-as-needed|--as-needed|target_link_libraries\s*\(' src/CMakeLists.txtLength of output: 2637
@olantwin Yes — it is still present.
The README still refers to a nonexistent --whole-archive block. The actual mechanism in src/CMakeLists.txt is:
- add the subsystem target to
target_link_libraries(SHiPGeometry ...); and - retain it at consumer link time through
target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed").
The documentation should be updated to reference that subsystem-target list and the --no-as-needed retention behavior. The earlier “addressed” status was incorrect.
b7d33d4 to
99a36d4
Compare
docs(readme): document self-registration and single-subsystem builds
99a36d4 to
6bddf29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
182-193: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
NeutrinoDetectorin the structure tree.It is listed as an implemented subsystem above and participates in the registry, but is missing from this repository layout. This makes the documented subsystem structure incomplete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 182 - 193, Update the README repository structure tree under subsystems to include the implemented NeutrinoDetector directory, keeping it alongside the other detector subsystem entries and preserving the documented layout.
♻️ Duplicate comments (1)
include/SHiPGeometry/SubsystemRegistry.h (1)
8-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing
<utility>include forstd::move(duplicate of prior review).
std::moveis used on line 56 but<utility>is not included. The header is not self-contained. This was previously flagged and remains unresolved.Proposed fix
`#include` <functional> `#include` <map> `#include` <string> +#include <utility> `#include` <vector>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/SHiPGeometry/SubsystemRegistry.h` around lines 8 - 11, Update the includes in SubsystemRegistry.h to add the standard <utility> header, ensuring the std::move usage in the registry implementation is declared and the header remains self-contained.
🧹 Nitpick comments (1)
src/CMakeLists.txt (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding
--no-as-neededfor non-GNU linkers.
-Wl,--no-as-neededis specific to GNU ld and gold. LLD supports it with GNU-compatible defaults, but Apple'slddoes not. If the project only targets Linux with GNU ld this is fine; otherwise consider a linker check:# Only apply on GNU-compatible linkers if(CMAKE_LINKER_ID STREQUAL "GNU" OR CMAKE_LINKER_ID STREQUAL "Gold") target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed") endif()This is a minor portability note — skip if Linux/GNU-only is an established constraint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/CMakeLists.txt` around lines 28 - 34, Guard the --no-as-needed option on target_link_options(SHiPGeometry) using a CMAKE_LINKER_ID check for GNU-compatible linkers such as GNU or Gold, leaving the option unset for unsupported linkers like Apple ld.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/SHiPGeometry/SubsystemRegistry.h`:
- Around line 54-58: Update registerSubsystem to inspect the insertion result
from registry().emplace. When desc.name is already registered, emit a diagnostic
or throw instead of silently discarding the new factory; preserve the successful
registration and true return behavior for unique names.
---
Outside diff comments:
In `@README.md`:
- Around line 182-193: Update the README repository structure tree under
subsystems to include the implemented NeutrinoDetector directory, keeping it
alongside the other detector subsystem entries and preserving the documented
layout.
---
Duplicate comments:
In `@include/SHiPGeometry/SubsystemRegistry.h`:
- Around line 8-11: Update the includes in SubsystemRegistry.h to add the
standard <utility> header, ensuring the std::move usage in the registry
implementation is declared and the header remains self-contained.
---
Nitpick comments:
In `@src/CMakeLists.txt`:
- Around line 28-34: Guard the --no-as-needed option on
target_link_options(SHiPGeometry) using a CMAKE_LINKER_ID check for
GNU-compatible linkers such as GNU or Gold, leaving the option unset for
unsupported linkers like Apple ld.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8c5f1e5-fedc-4a7e-9bc3-bf827e1168d9
📒 Files selected for processing (26)
Geometry]:README.mdapps/build_geometry.cppinclude/SHiPGeometry/SubsystemRegistry.hsrc/CMakeLists.txtsrc/SHiPGeometry.cppsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Cavern/include/Cavern/CavernFactory.hsubsystems/Cavern/src/CavernFactory.cppsubsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/Magnet/include/Magnet/MagnetFactory.hsubsystems/Magnet/src/MagnetFactory.cppsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.hsubsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cppsubsystems/Target/include/Target/TargetFactory.hsubsystems/Target/src/TargetFactory.cppsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.hsubsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
✅ Files skipped from review due to trivial changes (2)
- subsystems/Trackers/src/TrackersFactory.cpp
- subsystems/Cavern/src/CavernFactory.cpp
🚧 Files skipped from review as they are similar to previous changes (20)
- subsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.h
- subsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cpp
- subsystems/Magnet/include/Magnet/MagnetFactory.h
- subsystems/Calorimeter/src/CalorimeterFactory.cpp
- subsystems/MuonShield/include/MuonShield/MuonShieldFactory.h
- subsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.h
- subsystems/Cavern/include/Cavern/CavernFactory.h
- subsystems/Trackers/include/Trackers/TrackersFactory.h
- subsystems/DecayVolume/src/DecayVolumeFactory.cpp
- subsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.h
- subsystems/Target/include/Target/TargetFactory.h
- subsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.h
- subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
- subsystems/Magnet/src/MagnetFactory.cpp
- subsystems/Target/src/TargetFactory.cpp
- subsystems/MuonShield/src/MuonShieldFactory.cpp
- subsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.h
- apps/build_geometry.cpp
- subsystems/TimingDetector/src/TimingDetectorFactory.cpp
- src/SHiPGeometry.cpp
…tion registerSubsystem inspected nothing from emplace(), so a second subsystem declaring an already-registered descriptor name was silently discarded and registration still reported success. Check the insertion result and abort with a diagnostic naming the offending subsystem; registration runs at static-init, so a throw would reach no handler. Add <cstdio>, <cstdlib> and <utility> so the header is self-contained, and correct the REGISTER_SUBSYSTEM note, which still described the subsystems as static archives needing --whole-archive. Restrict -Wl,--no-as-needed to non-Apple UNIX, and add NeutrinoDetector to the README structure tree.
|
@CodeRabbit full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 36 minutes. |
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/CMakeLists.txt`:
- Around line 28-38: Remove the unconditional target_link_options call for
SHiPGeometry, leaving only the --no-as-needed option inside the existing
“if(UNIX AND NOT APPLE)” guard. Preserve the guarded platform-specific linker
behavior.
In `@subsystems/Magnet/include/Magnet/MagnetFactory.h`:
- Around line 31-34: Update MagnetFactory::descriptor so its placement envelope
no longer overlaps the TrackersFactory envelope when both are assembled as world
siblings; preserve the intentional shared z-center while adjusting the Magnet
envelope’s z extent or placement to fit outside the Trackers span. Verify the
resulting descriptor bounds are non-intersecting.
In `@subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp`:
- Around line 145-146: Update the registry integration for UpstreamTaggerFactory
so the SHiPUBTManager* reaches build() when constructed through
REGISTER_SUBSYSTEM, allowing setContainerVolume() to execute. Modify the
relevant factory registration/build path rather than the unrelated
container-volume logic, and preserve direct construction behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26b0db48-46bf-4ac2-8c71-09f8ce040c1f
📒 Files selected for processing (26)
Geometry]:README.mdapps/build_geometry.cppinclude/SHiPGeometry/SubsystemRegistry.hsrc/CMakeLists.txtsrc/SHiPGeometry.cppsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Cavern/include/Cavern/CavernFactory.hsubsystems/Cavern/src/CavernFactory.cppsubsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/Magnet/include/Magnet/MagnetFactory.hsubsystems/Magnet/src/MagnetFactory.cppsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.hsubsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cppsubsystems/Target/include/Target/TargetFactory.hsubsystems/Target/src/TargetFactory.cppsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.hsubsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
|
One general comment: let's use spdlog for logging. I didn't catch it in the original commits. |
|
Full review coming... |
|
There are still quite a few open code rabbit comments and a merge conflict. What's your plan for this PR? |
|
I was waiting for your review, the main features are there, the coderabbit stuff may lead to refactors which should only be done once we agree about the way this should be merged. |
|
Ok, then I'll review now. |
olantwin
left a comment
olantwin
left a comment
There was a problem hiding this comment.
A few questions and nitpicks.
There was a problem hiding this comment.
This file should be removed.
Please add only files (or better: lines/hunks) you intend to commit.
| # link line, which is where the entries were being dropped. | ||
| target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed") | ||
|
|
||
| if(UNIX AND NOT APPLE) |
There was a problem hiding this comment.
This unguarded version probably needs to be removed.
| 4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`, and | ||
| add the new library to the `--whole-archive` block in `src/CMakeLists.txt` | ||
| so its self-registration initializer is not dropped by the linker |
There was a problem hiding this comment.
This issue seems to still be present?
| # The subsystems are referenced only through their REGISTER_SUBSYSTEM static | ||
| # initialisers, so the toolchain's default --as-needed drops them from the | ||
| # DT_NEEDED of every executable that links SHiPGeometry, and their | ||
| # registrations never run. INTERFACE puts --no-as-needed on the *consumer's* | ||
| # link line, which is where the entries were being dropped. |
There was a problem hiding this comment.
This probably only works for dynamically linked builds. Maybe we should disallow static builds or raise a warning if build statically.
| #include "DecayVolume/DecayVolumeFactory.h" | ||
| #include "Magnet/MagnetFactory.h" | ||
| #include "MuonShield/MuonShieldFactory.h" | ||
| #include "SHiPGeometry/Placement.h" |
There was a problem hiding this comment.
I don't think we use this header anywhere anymore. Maybe time to delete it entirely?
| namespace { | ||
| bool endsWith(const std::string& s, const std::string& suffix) { | ||
| return s.size() >= suffix.size() && | ||
| s.compare(s.size() - suffix.size(), suffix.size(), suffix) == 0; | ||
| } | ||
| } // namespace |
There was a problem hiding this comment.
This isn't needed, use std::string::ends_with()
|
|
||
| } // namespace | ||
|
|
||
| GeoPhysVol* assembleGeometry(const std::vector<std::string>& only) { |
There was a problem hiding this comment.
Is the selection parameter used anywhere (e.g. the CI)?
| #include "Target/TargetFactory.h" | ||
| #include "TimingDetector/TimingDetectorFactory.h" | ||
| #include "Trackers/TrackersFactory.h" | ||
| #include "UpstreamTagger/SHiPUBTManager.h" |
There was a problem hiding this comment.
I think SHiPUBTManager is also now useless.
Sidenote: Let's not add SHiP to our classnames... I know it's my fault for some as well. Probably something deserving of a sweeping cleanup.
olantwin
left a comment
olantwin
left a comment
There was a problem hiding this comment.
Some unaddressed coderabbit comments and some new comments from me.
| elseif(NOT BUILD_SHARED_LIBS) | ||
| # Static build: --no-as-needed does not apply to archives. Subsystem | ||
| # self-registration would instead need --whole-archive (or equivalent), | ||
| # which is not configured here, so the registry may be empty at run time. | ||
| message( | ||
| WARNING | ||
| "Static build detected: subsystem self-registration relies on " | ||
| "-Wl,--no-as-needed for shared libraries and is not wired up for static " | ||
| "archives. build_geometry/tests may find an empty subsystem registry. " | ||
| "Prefer BUILD_SHARED_LIBS=ON." | ||
| ) |
There was a problem hiding this comment.
This check needs to come first and can't be exclusive with Linux (unix and not apple)
| 4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`, and | ||
| add the new library to the `--whole-archive` block in `src/CMakeLists.txt` | ||
| so its self-registration initializer is not dropped by the linker |
|
|
||
| /// This subsystem's self-description (name, node, id, placement). | ||
| static SubsystemDescriptor descriptor() { | ||
| return {"Trackers", "/SHiP/trackers", 5, 0.0, 0.0, 89570.0, false}; |
There was a problem hiding this comment.
Can we use s_containerCentreZ here instead of hardcoding the same number?
| if (manager) { | ||
| manager->setContainerVolume(containerPhys); | ||
| } | ||
| manager->setContainerVolume(containerPhys); |
There was a problem hiding this comment.
Maybe here the previous method was better, at least i don't see the advantage of adding a dummy manager over skipping the setting of the container volume.
| #include "UpstreamTagger/UpstreamTaggerFactory.h" | ||
|
|
||
| #include "SHiPGeometry/SHiPMaterials.h" | ||
| #include "SHiPGeometry/SubsystemRegistry.h" |
There was a problem hiding this comment.
Here (and elsewhere): this is a pretty heavy header. Most isn't needed in the factory headers. I suggest we split this so that a light-weight header is included in the factory headers and the rest in the factory implementations.
| for (auto& entry : reg) { | ||
| if (entry.second.desc.isWorld) { | ||
| world = dynamic_cast<GeoPhysVol*>(entry.second.build(materials)); | ||
| break; | ||
| } |
There was a problem hiding this comment.
- Using structured bindings would make this more readable
- Is the cast here reliable? I don't have the inheritance structure of GeoPhysVol in my head right now, but can this over/undermatch?
- build_geometry: assemble a selection of subsystems (>1 name) instead of rejecting it; reinstates assembleGeometry(only) with name validation - geometry: split SubsystemDescriptor into a lightweight header so the factory headers no longer pull in the full registry (map, functional, GeoPhysVol); the registry machinery stays in SubsystemRegistry.h - geometry: validate the world dynamic_cast and use structured bindings in the assembler, so a mis-typed world fails clearly - upstream-tagger: revert the local-manager fallback and keep the manager guard (a local manager is destroyed at scope exit, so it did nothing); fixes an unconditional deref - trackers: derive descriptor z from s_containerCentreZ instead of the hardcoded 89570 - cmake: check static builds first and independently of the Linux guard, warn that self-registration needs shared libs - readme: fix the stale --whole-archive instruction, add NeutrinoDetector
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 37 minutes. |
…r after merge issues were resolved
2 participants
feat(apps): build individual subsystems from build_geometry
build_geometry now accepts a subsystem name and builds that subsystem on
its own (local frame), alongside the default full-detector build:
build_geometry # full detector -> ship_geometry.db
build_geometry Calorimeter # single subsystem -> Calorimeter.db
build_geometry --list # list subsystem names
A token ending in .db is treated as the output file; any other token names
a subsystem, as spelled by --list. The standalone build_subsystem
executable is removed and its behaviour folded into build_geometry.
Update the README build, factory-pattern, structure, and "adding a
subsystem" sections to match.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
build_geometrysupport to list subsystem names, build the full detector by default, or build a single named subsystem with configurable.dboutput.Bug Fixes
Documentation
gmexviewing steps.Chores
It is now possible to build (and visualise, although the proper visualisation will be the subject of another PR) single or incomplete sets of detectors, for example the target and the Magnet: