updated with calorimeter, to be checked#11
Open
matclim wants to merge 10 commits intoShipSoft:mainfrom
Open
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:
📝 WalkthroughWalkthroughThis PR introduces a config-driven calorimeter geometry system. It adds material definitions (Lead, PVT, Polystyrene), defines a CalorimeterConfig struct and TOML parser, creates modular layer placement utilities, refactors CalorimeterFactory to consume the config, integrates tomlplusplus into the build system, and extends tests to validate config-driven Z-height calculations. ChangesCalorimeter Config-Driven Geometry Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🧹 Nitpick comments (1)
subsystems/Calorimeter/test_calorimeter.cpp (1)
32-39: Make the child-count assertion stronger.
getNChildVols() >= 1only proves the container is non-empty. This refactor can still drop most layers/modules and keep the test green. Please assert the expected count from the stagedcalo.cfg, or at least a lower bound derived frommodule_nx * module_ny * populated_layer_count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystems/Calorimeter/test_calorimeter.cpp` around lines 32 - 39, Update the test CalorimeterHasChildren to assert a stronger expected child count: instead of CHECK(calo->getNChildVols() >= 1u), compute or hardcode the expected lower bound from the staged configuration and assert equality or a tighter >=, e.g. read module_nx, module_ny and populated_layer_count (or the expected total layers from calo.cfg) and assert calo->getNChildVols() >= module_nx * module_ny * populated_layer_count (or == expected_total_children); modify the test to access SHiPMaterials/CalorimeterFactory configuration values (or parse calo.cfg) and replace the loose check on calo->getNChildVols() with the computed stronger assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/SHiPMaterials.cpp`:
- Around line 194-208: The PVT and Polystyrene materials are being added with
raw atom counts (stoichiometry) instead of normalized mass fractions; update the
GeoMaterial creation for "PVT" and "Polystyrene" to compute mass fractions from
atomic masses (use m_elements["Carbon"] and m_elements["Hydrogen"] to get each
element's atomic weight), compute the molecule molecular weight (e.g., MW =
9*AW_C + 10*AW_H for PVT), then add normalized fractions (count*AW_element / MW)
to the GeoMaterial before lock(), and keep storing them in m_materials["PVT"]
and m_materials["Polystyrene"] as before.
In `@subsystems/Calorimeter/CMakeLists.txt`:
- Around line 34-43: The compile-time fallback CALO_CFG_DEFAULT_PATH currently
embeds the source-tree path but the installed calo.cfg is not probed; update the
build and runtime lookup so installed data is found: add a second compile
definition (or an exported header) that contains the install data directory path
(the ${CMAKE_INSTALL_DATADIR}/SHiPGeometry location) when installing the
Calorimeter target, and modify resolveCfgPath() in CalorimeterFactory.cpp to
check that install-time data-dir path in addition to the working directory and
CALO_CFG_DEFAULT_PATH (use the unique symbol CALO_CFG_DEFAULT_PATH and the
resolveCfgPath function name to locate the places to change). Ensure the
install-time path is only set when performing an install so local development
still falls back to the source-tree path.
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 261-315: The loop in CalorimeterFactory.cpp that iterates
cfg.layers2 accepts codes 1,2,5,6,7,8 but not 3 or 4, causing a runtime throw
while totalStackZ() treats 1..4 as valid; fix by either (A) adding explicit
branches for codes 3 and 4 that create the expected thin-PS scintillator layers
(mirror the pattern used for code 1/2: build an env via makeEnv and call the
appropriate placement/build routine, increment zCursor and indices like sl/gl),
or (B) validate/normalize cfg.layers2 earlier (e.g. in totalStackZ() caller or
CalorimeterFactory constructor) to reject or translate codes 3 and 4 and throw a
clear config error; reference the loop over cfg.layers2, totalStackZ(), makeEnv,
CaloBarLayer::place and CaloFibreHPLayer::build when making the change.
- Around line 83-107: Before placing modules, validate that the calorimeter
stack and tiled footprint fit inside the fixed container: compute total Z extent
using totalStackZ(cfg) and compute full X/Y extents from cfg.module_nx,
cfg.module_ny and effective pitch (use same logic as pitchX/pitchY:
cfg.module_pitch_x_mm>0 ? cfg.module_pitch_x_mm : cfg.plate_xy_mm), then compare
half-extents against s_containerHalfZ and s_containerHalfX/s_containerHalfY
respectively; if any extent exceeds the container, throw a clear runtime_error
(or appropriate config exception) with a descriptive message referencing the
offending dimension and the computed vs allowed size before the loop that calls
buildStack(containerPhys, cfg, ...).
---
Nitpick comments:
In `@subsystems/Calorimeter/test_calorimeter.cpp`:
- Around line 32-39: Update the test CalorimeterHasChildren to assert a stronger
expected child count: instead of CHECK(calo->getNChildVols() >= 1u), compute or
hardcode the expected lower bound from the staged configuration and assert
equality or a tighter >=, e.g. read module_nx, module_ny and
populated_layer_count (or the expected total layers from calo.cfg) and assert
calo->getNChildVols() >= module_nx * module_ny * populated_layer_count (or ==
expected_total_children); modify the test to access
SHiPMaterials/CalorimeterFactory configuration values (or parse calo.cfg) and
replace the loose check on calo->getNChildVols() with the computed stronger
assertion.
🪄 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: 33c8398b-10cc-494d-a272-1a3039478fd2
📒 Files selected for processing (11)
.clang-format.github/dependabot.yml.github/workflows/build-test.yml.gitignore.pre-commit-config.yamlsrc/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cpp
💤 Files with no reviewable changes (5)
- .github/dependabot.yml
- .clang-format
- .gitignore
- .github/workflows/build-test.yml
- .pre-commit-config.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 185-197: layerId is reset to 0 inside buildStack(), so
GeoIdentifierTag instances created by makeEnv are duplicated across modules
(mx,my) when all layers are placed under the same container; change the code so
identifiers are globally unique per module by either (A) making a per-module
parent GeoPhysVol and attach layers to that parent instead of the shared
container, or (B) incorporate module coordinates into the identifier before
creating GeoIdentifierTag (e.g. compute a base offset using mx,my and add local
layerId), updating references in build(), buildStack(), makeEnv, and any code
that reads GeoIdentifierTag so it expects the encoded identifier; ensure layerId
incrementing still produces unique values and apply same fix to the other
occurrence around lines ~293-296.
🪄 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: 0173ed9e-1d2f-4c37-94a7-31da7b805cc1
📒 Files selected for processing (3)
src/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/src/CalorimeterFactory.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/SHiPMaterials.cpp
- subsystems/Calorimeter/CMakeLists.txt
Author
|
I don't understand the pre-commit ci error ? |
Contributor
Because you deleted the pre-commit config file (and a bunch of other files!) in your PR. Please revert these unrelated changes. I'll review the calorimeter changes tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystems/Calorimeter/src/CalorimeterFactory.cpp`:
- Around line 269-271: The ECAL branch for code == 8 updates zCursor but does
not increment gl, while the HCAL branch increments gl; make the behavior
explicit and consistent: either add "++gl" immediately after "zCursor +=
airGapZ;" in the ECAL branch to match HCAL, or (if air gaps intentionally count
as geometry layers only for HCAL) add a short clarifying comment in both
branches explaining the difference; update the code paths handling code == 8
(zCursor, airGapZ, gl) in CalorimeterFactory.cpp accordingly so the intent is
unambiguous.
- Around line 185-200: layerId is reset per buildStack(), causing duplicate
GeoIdentifierTag values across modules; modify the code so identifiers are
globally unique by either (A) making layerId a persistent counter (e.g. a member
or static counter used by buildStack()) or (B) encoding module indices into the
id when creating GeoIdentifierTag (e.g. compute id = mx*<scale1> + my*<scale2> +
layerId where <scale1>/<scale2> are chosen to avoid collisions); update the
makeEnv lambda to use the new id calculation or the shared counter when calling
container->add(new GeoIdentifierTag(...)) and keep GeoNameTag usage (mtag)
unchanged.
🪄 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: 4ed00c30-7d24-4015-87a0-de097a6e2244
📒 Files selected for processing (6)
src/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cpp
✅ Files skipped from review due to trivial changes (2)
- src/SHiPGeometry.cpp
- src/SHiPMaterials.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- subsystems/Calorimeter/CMakeLists.txt
Contributor
These files all seem to be missing. |
Contributor
|
Could you please add the missing files? |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystems/Calorimeter/src/CaloBarLayer.cpp`:
- Around line 18-27: In CaloBarLayer::place validate inputs up-front: check
tagPrefix is non-null before calling std::string(tagPrefix) (or accept empty
string), ensure barLog is non-null before constructing GeoPhysVol(barLog), and
ensure mother is non-null before calling mother->add(...); also validate nBars>0
and handle invalid values by throwing a descriptive std::invalid_argument or
logging an error and returning early. Include clear error messages naming the
failing parameter (e.g., "CaloBarLayer::place: null tagPrefix", "null barLog",
"null mother", "nBars <= 0") so callers can diagnose the cause.
In `@subsystems/Calorimeter/src/CaloFibreHPLayer.cpp`:
- Around line 39-55: The code in CaloFibreHPLayer attaches the casing to the
geometry tree (calls to mother->add with GeoNameTag/GeoTransform/casingPhys)
before validating fiberCoreDiam_mm (rCore), which can leave a partially built
geometry on error; move the validation of rCore (the check using
fiberCoreDiam_mm and rOuter) to the top of the function or at least before any
mother->add calls, or alternatively construct the casing and fibre volumes into
local temporary GeoPhysVol objects and only call mother->add(casingPhys) and
subsequent adds after the rCore validation succeeds so an exception cannot leave
a partially mutated geometry tree.
In `@subsystems/Calorimeter/src/CalorimeterConfig.cpp`:
- Around line 92-96: The "center_stack" branch currently lowercases val without
removing a trailing semicolon, so values like "true;" become "true;" and
cfg.center_stack is false; before transforming/normalizing val in the else if
(key == "center_stack") block, strip any trailing semicolon (same normalization
used for the numeric keys), then lowercase and set cfg.center_stack based on the
cleaned string (compare to "1", "true", "yes", "on") so cfg.center_stack is
parsed correctly; locate the branch handling key == "center_stack" and apply the
same trailing-semicolon trimming used elsewhere in CalorimeterConfig.cpp.
🪄 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: 2612f7e8-68b7-4cb6-800c-c0d880f9bc42
📒 Files selected for processing (6)
subsystems/Calorimeter/include/Calorimeter/CaloBarLayer.hsubsystems/Calorimeter/include/Calorimeter/CaloFibreHPLayer.hsubsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.hsubsystems/Calorimeter/src/CaloBarLayer.cppsubsystems/Calorimeter/src/CaloFibreHPLayer.cppsubsystems/Calorimeter/src/CalorimeterConfig.cpp
Comment on lines
+18
to
+27
| void CaloBarLayer::place(GeoVPhysVol* mother, | ||
| GeoLogVol* barLog, | ||
| double pitch_mm, | ||
| int nBars, | ||
| double zCenter_mm, | ||
| const char* tagPrefix, | ||
| int layerIndex, | ||
| BarAxis axis, | ||
| const std::string& nameSuffix) | ||
| { |
There was a problem hiding this comment.
Add defensive validation for API inputs before placement.
Line 39 (std::string(tagPrefix)) and Lines 44–47 (GeoPhysVol(barLog), mother->add(...)) assume all pointers are valid. A null tagPrefix, barLog, or mother will hard-fail at runtime; nBars <= 0 silently builds nothing.
Proposed fix
`#include` "Calorimeter/CaloBarLayer.h"
@@
`#include` <string>
+#include <stdexcept>
@@
void CaloBarLayer::place(GeoVPhysVol* mother,
GeoLogVol* barLog,
double pitch_mm,
int nBars,
@@
BarAxis axis,
const std::string& nameSuffix)
{
+ if (mother == nullptr) throw std::invalid_argument("CaloBarLayer::place: mother is null");
+ if (barLog == nullptr) throw std::invalid_argument("CaloBarLayer::place: barLog is null");
+ if (tagPrefix == nullptr) throw std::invalid_argument("CaloBarLayer::place: tagPrefix is null");
+ if (nBars <= 0) throw std::invalid_argument("CaloBarLayer::place: nBars must be > 0");
+
const double pitch = pitch_mm * mm;
const double s0 = -0.5 * (nBars - 1) * pitch;Also applies to: 38-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystems/Calorimeter/src/CaloBarLayer.cpp` around lines 18 - 27, In
CaloBarLayer::place validate inputs up-front: check tagPrefix is non-null before
calling std::string(tagPrefix) (or accept empty string), ensure barLog is
non-null before constructing GeoPhysVol(barLog), and ensure mother is non-null
before calling mother->add(...); also validate nBars>0 and handle invalid values
by throwing a descriptive std::invalid_argument or logging an error and
returning early. Include clear error messages naming the failing parameter
(e.g., "CaloBarLayer::place: null tagPrefix", "null barLog", "null mother",
"nBars <= 0") so callers can diagnose the cause.
Author
|
I'm not too sure what is wrong with the ci? It says something about gersemi not finding the rust compiler? |
Contributor
That's unrelated, fixed already in FairShip, but forgot to also apply the fix here. |
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
subsystems/Calorimeter/src/CaloFibreHPLayer.cpp (1)
31-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidation occurs after geometry mutation.
The casing is attached to
motherat Lines 36-38 before the fibre diameter validation at Lines 44-47. IfrCore <= 0orrCore > rOuter, the function throws but leaves a partially built HPL (the casing) in the geometry tree. Move the validation to before the firstmother->add(...)call.Proposed fix
void CaloFibreHPLayer::build(GeoVPhysVol* mother, GeoMaterial* aluminiumMat, GeoMaterial* fibreMat, const std::string& layerTag, double zCenter_mm, int layerIndex, double casingXY_mm, double casingZ_mm, double fiberDiam_mm, double fiberCoreDiam_mm, bool fibresAlongY, const std::string& nameSuffix) { const double casingXY = casingXY_mm * mm; const double casingZ = casingZ_mm * mm; + // ── validate fibre geometry before mutating the tree ────────────────── + const double rOuter = 0.5 * fiberDiam_mm * mm; + const double rCore = 0.5 * fiberCoreDiam_mm * mm; + + if (rCore <= 0.0 || rCore > rOuter) + throw std::runtime_error( + "CaloFibreHPLayer: invalid core diameter " + "(must be >0 and <= outer diameter)"); + // ── aluminium casing ────────────────────────────────────────────────── auto* casingShape = new GeoBox(0.5 * casingXY, 0.5 * casingXY, 0.5 * casingZ); auto* casingLog = new GeoLogVol("HPL_CasingLog", casingShape, aluminiumMat); auto* casingPhys = new GeoPhysVol(casingLog); mother->add(new GeoNameTag((layerTag + "_HPL_Casing" + nameSuffix).c_str())); mother->add(new GeoTransform(GeoTrf::Translate3D(0.0, 0.0, zCenter_mm * mm))); mother->add(casingPhys); - // ── fibre geometry ──────────────────────────────────────────────────── - const double rOuter = 0.5 * fiberDiam_mm * mm; - const double rCore = 0.5 * fiberCoreDiam_mm * mm; - - if (rCore <= 0.0 || rCore > rOuter) - throw std::runtime_error( - "CaloFibreHPLayer: invalid core diameter " - "(must be >0 and <= outer diameter)"); - // GeoTube axis is Z; rotate so fibres run along Y or X🤖 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/Calorimeter/src/CaloFibreHPLayer.cpp` around lines 31 - 47, The validation of fibre radii (rCore and rOuter) is happening after the casing geometry (casingShape/casingLog/casingPhys) is already attached to the geometry tree via mother->add; move the check for rCore <= 0 or rCore > rOuter to before any mutation of mother (i.e., before creating/adding casingShape/casingLog/casingPhys and before the first mother->add call) so that the function throws without leaving a partially-built GeoPhysVol in the tree; ensure the variables rOuter and rCore are computed prior to that validation and only proceed to construct and add the GeoBox/GeoLogVol/GeoPhysVol and GeoNameTag/GeoTransform when the validation passes.
🤖 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 `@subsystems/Calorimeter/test_calorimeter.cpp`:
- Line 4: The test build fails because test_calorimeter.cpp includes
"Calorimeter/CalorimeterConfig.h" but the header is not visible to the test
target; fix by wiring the Calorimeter library as a dependency or exporting its
include directories to the test target: ensure the test CMake target calls
target_link_libraries(<test target> PRIVATE|PUBLIC Calorimeter) or
target_include_directories(<test target> PRIVATE
$<TARGET_PROPERTY:Calorimeter,INTERFACE_INCLUDE_DIRECTORIES>) so that
CalorimeterConfig.h (from the Calorimeter target) is available; alternatively,
if the header is supposed to be local to the test, change the include to a
relative path to the header, but prefer linking the Calorimeter target so
symbols like CalorimeterConfig.h resolve consistently in test builds.
---
Duplicate comments:
In `@subsystems/Calorimeter/src/CaloFibreHPLayer.cpp`:
- Around line 31-47: The validation of fibre radii (rCore and rOuter) is
happening after the casing geometry (casingShape/casingLog/casingPhys) is
already attached to the geometry tree via mother->add; move the check for rCore
<= 0 or rCore > rOuter to before any mutation of mother (i.e., before
creating/adding casingShape/casingLog/casingPhys and before the first
mother->add call) so that the function throws without leaving a partially-built
GeoPhysVol in the tree; ensure the variables rOuter and rCore are computed prior
to that validation and only proceed to construct and add the
GeoBox/GeoLogVol/GeoPhysVol and GeoNameTag/GeoTransform when the validation
passes.
🪄 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: 30df109d-6c5d-4fcf-9726-2f8c29b95dc2
📒 Files selected for processing (12)
src/SHiPGeometry.cppsrc/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/include/Calorimeter/CaloBarLayer.hsubsystems/Calorimeter/include/Calorimeter/CaloFibreHPLayer.hsubsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.hsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CaloBarLayer.cppsubsystems/Calorimeter/src/CaloFibreHPLayer.cppsubsystems/Calorimeter/src/CalorimeterConfig.cppsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cpp
✅ Files skipped from review due to trivial changes (1)
- src/SHiPMaterials.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/SHiPGeometry.cpp
- subsystems/Calorimeter/CMakeLists.txt
| // SPDX-License-Identifier: LGPL-3.0-or-later | ||
| // Copyright (C) CERN for the benefit of the SHiP Collaboration | ||
|
|
||
| #include "Calorimeter/CalorimeterConfig.h" |
There was a problem hiding this comment.
Build blocker: CalorimeterConfig.h is not resolvable in this test target.
Static analysis reports a hard compile error here ('Calorimeter/CalorimeterConfig.h' file not found). Please fix the target include/export wiring (preferred) or adjust the include path so this header resolves consistently in test builds.
🧰 Tools
🪛 Clang (14.0.6)
[error] 4-4: 'Calorimeter/CalorimeterConfig.h' file not found
(clang-diagnostic-error)
🤖 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/Calorimeter/test_calorimeter.cpp` at line 4, The test build fails
because test_calorimeter.cpp includes "Calorimeter/CalorimeterConfig.h" but the
header is not visible to the test target; fix by wiring the Calorimeter library
as a dependency or exporting its include directories to the test target: ensure
the test CMake target calls target_link_libraries(<test target> PRIVATE|PUBLIC
Calorimeter) or target_include_directories(<test target> PRIVATE
$<TARGET_PROPERTY:Calorimeter,INTERFACE_INCLUDE_DIRECTORIES>) so that
CalorimeterConfig.h (from the Calorimeter target) is available; alternatively,
if the header is supposed to be local to the test, change the include to a
relative path to the header, but prefer linking the Calorimeter target so
symbols like CalorimeterConfig.h resolve consistently in test builds.
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
|
@matclim could you please update the global and subsystem readme? |
Contributor
|
Could you please update the commit message or suggest a commit message if you'd like this to be squashed? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
subsystems/Calorimeter/src/CaloBarLayer.cpp (1)
18-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNull-pointer and invalid-input validation still absent (previously raised).
The past review flagged that
mother,barLog, andtagPrefixare dereferenced at lines 33, 36-39 without null-checks, and thatnBars <= 0is silently ignored. The code is unchanged in this regard.🤖 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/Calorimeter/src/CaloBarLayer.cpp` around lines 18 - 40, The CaloBarLayer::place function must validate inputs before use: check that mother, barLog, and tagPrefix are not null and that nBars > 0; if any check fails, either throw a std::invalid_argument (or log and return) with a clear message referencing the failing parameter. Add these guards at the top of CaloBarLayer::place (before any dereferencing or loop) and include the function name in the error text so callers can trace failures; keep existing behavior unchanged otherwise (still create GeoNameTag/GeoTransform/GeoPhysVol when inputs are valid).
🧹 Nitpick comments (1)
subsystems/Calorimeter/src/CaloBarLayer.cpp (1)
6-10: ⚡ Quick winAdd a direct
#includeforGeoVPhysVol.h.
mother->add(...)requires the full definition ofGeoVPhysVol, which is currently satisfied only through a transitive include viaGeoPhysVol.h. Any header reorganisation in GeoModelKernel would silently break this TU.♻️ Proposed fix
`#include` <GeoModelKernel/GeoLogVol.h> `#include` <GeoModelKernel/GeoNameTag.h> `#include` <GeoModelKernel/GeoPhysVol.h> `#include` <GeoModelKernel/GeoTransform.h> +#include <GeoModelKernel/GeoVPhysVol.h> `#include` <GeoModelKernel/Units.h>🤖 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/Calorimeter/src/CaloBarLayer.cpp` around lines 6 - 10, The translation unit uses GeoVPhysVol (via calls like mother->add(...)) but only relies on a transitive include through GeoPhysVol.h; add a direct `#include` for GeoVPhysVol.h so the full definition of GeoVPhysVol is guaranteed (e.g., insert `#include` <GeoModelKernel/GeoVPhysVol.h>) to avoid breakage if GeoModelKernel headers are reorganized.
🤖 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 `@subsystems/Calorimeter/src/CalorimeterConfig.cpp`:
- Around line 152-155: The code currently assigns cfg.module_nx and
cfg.module_ny by truncating the numeric result of readNumeric; instead, when
table["module_nx"] or table["module_ny"] is present call readNumeric to get a
numeric value, verify it is an integer (e.g. value == std::floor(value)) and
strictly positive (> 0), and if the check fails reject the config by throwing or
returning an error rather than casting; update the parsing code that references
table["module_nx"], table["module_ny"], cfg.module_nx, cfg.module_ny and
readNumeric to perform these checks and produce a clear error message on
failure.
---
Duplicate comments:
In `@subsystems/Calorimeter/src/CaloBarLayer.cpp`:
- Around line 18-40: The CaloBarLayer::place function must validate inputs
before use: check that mother, barLog, and tagPrefix are not null and that nBars
> 0; if any check fails, either throw a std::invalid_argument (or log and
return) with a clear message referencing the failing parameter. Add these guards
at the top of CaloBarLayer::place (before any dereferencing or loop) and include
the function name in the error text so callers can trace failures; keep existing
behavior unchanged otherwise (still create GeoNameTag/GeoTransform/GeoPhysVol
when inputs are valid).
---
Nitpick comments:
In `@subsystems/Calorimeter/src/CaloBarLayer.cpp`:
- Around line 6-10: The translation unit uses GeoVPhysVol (via calls like
mother->add(...)) but only relies on a transitive include through GeoPhysVol.h;
add a direct `#include` for GeoVPhysVol.h so the full definition of GeoVPhysVol is
guaranteed (e.g., insert `#include` <GeoModelKernel/GeoVPhysVol.h>) to avoid
breakage if GeoModelKernel headers are reorganized.
🪄 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: b978e91c-2495-42c8-87af-2a4ca440edf1
📒 Files selected for processing (12)
src/SHiPMaterials.cppsubsystems/Calorimeter/CMakeLists.txtsubsystems/Calorimeter/calo.tomlsubsystems/Calorimeter/include/Calorimeter/CaloBarLayer.hsubsystems/Calorimeter/include/Calorimeter/CaloFibreHPLayer.hsubsystems/Calorimeter/include/Calorimeter/CalorimeterConfig.hsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CaloBarLayer.cppsubsystems/Calorimeter/src/CaloFibreHPLayer.cppsubsystems/Calorimeter/src/CalorimeterConfig.cppsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Calorimeter/test_calorimeter.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- subsystems/Calorimeter/CMakeLists.txt
- src/SHiPMaterials.cpp
- subsystems/Calorimeter/src/CalorimeterFactory.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check that it works as expected, tests are passed but I am not sure what they entail
Summary by CodeRabbit
Release Notes
New Features
Tests