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 span-based iteration types for boxed array regions, span-aware FieldBox utilities (operate_on_span, operate_on_fields, fill_ghost), extends GridLayout with domainBoxFor, mutability/const-correctness changes to Span, and new tests plus CMake wiring for field box span tests. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
PhilipDeegan
force-pushed
the
new_box_iter
branch
from
5ac593e to
6a029b8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| TYPED_TEST_SUITE(FieldBoxSpanTest, Permutations_t, ); | ||
|
|
||
| TYPED_TEST(FieldBoxSpanTest, test_field_box_span_nd) |
Check notice
Code scanning / CodeQL
Unused static variable Note test
PhilipDeegan
force-pushed
the
new_box_iter
branch
from
6a029b8 to
5c1a022
Compare
![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 (4)
src/core/data/field/field_box.hpp (1)
64-69:sizeof(double)is hardcoded — fragile even within theEquals<double>constraint.Both constrained overloads hard-code the element width:
std::memcpy(dst.data(), src.data(), src.size() * sizeof(double)); // line 68 std::memcpy(dst.data(), src_data, dst.size() * sizeof(double)); // line 115While the
requires(std::is_same_v<Operator, Equals<double>>)constraint ties these to thedoubleoperator today, the actual span value type should be used so the code remains correct if the field type changes (e.g., tofloator a custom scalar):♻️ Proposed refactor
- std::memcpy(dst.data(), src.data(), src.size() * sizeof(double)); + using vt = std::remove_reference_t<decltype(*dst.data())>; + std::memcpy(dst.data(), src.data(), src.size() * sizeof(vt));- std::memcpy(dst.data(), src_data, dst.size() * sizeof(double)); + using vt = std::remove_reference_t<decltype(*dst.data())>; + std::memcpy(dst.data(), src_data, dst.size() * sizeof(vt));Also applies to: 111-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/field/field_box.hpp` around lines 64 - 69, The memcpy size is hardcoded to sizeof(double) in operate_on_field_row (and the other overload) which is fragile; replace sizeof(double) with the span element size by extracting the span value_type from the dst (or src) parameter (e.g. using T = std::remove_reference_t<decltype(dst)>::value_type) and use sizeof(T) for the memcpy byte count; update both occurrences (the memcpy in operate_on_field_row and the memcpy in the other overload that copies from src_data into dst) so they compute the copy size from the span element type instead of assuming double, keeping the Equals<double> constraint but making the code robust to different span element types.src/core/data/field/field_box_span.hpp (2)
26-26: Prefernullptrover0for pointer initialization.
Span<value_type> row{0, 0}uses integer0as a null pointer constant. While valid,nullptris more idiomatic in modern C++.Suggested fix
- Span<value_type> row{0, 0}; + Span<value_type> row{nullptr, 0};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/field/field_box_span.hpp` at line 26, Replace the integer null constants used to initialize the Span with C++11 nullptr: locate the declaration Span<value_type> row{0, 0} and change the initializer elements from 0 to nullptr so the span's pointer members are initialized with a proper null pointer (e.g., Span<value_type> row{nullptr, nullptr}); ensure this change is applied wherever the Span<value_type> row variable or similar Span initializations appear.
51-89: Dual iterator/range pattern forFieldBoxSlabis correct but worth documenting.
FieldBoxSlabserves as both the outer iterator (with++/!=/*) and the inner range (withbegin()/end()returningFieldBoxRows). This is clever and works correctly in nested range-for loops, but a brief comment explaining this dual role would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/field/field_box_span.hpp` around lines 51 - 89, Add a short explanatory comment inside the FieldBoxSlab template describing its dual iterator/range role: that FieldBoxSlab acts as the outer iterator (providing operator++, operator==/!= and operator* to return itself) while also acting as an inner range by exposing begin()/end() that return FieldBoxRows<Array_t>, enabling nested range-for usage; mention the relevant symbols FieldBoxSlab, begin(), end(), operator++(), operator*(), and FieldBoxRows to guide future readers.src/core/utilities/span.hpp (1)
41-48: Consider returningT const*fromconstaccessors, or document thestd::span-like const model.
data() const/begin() const/end() constall returnT*(non-const pointer) — even on aconst Span. This is intentionalstd::span-like behavior (constness of the element is encoded inT, not the span), but it can surprise users who expect aconst Span<double>to prevent mutation.Also,
size_address()exposes the internal size member by pointer — consider adding a brief comment on its intended use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/utilities/span.hpp` around lines 41 - 48, The const-qualified accessors data() const, begin() const and end() const currently return T* which allows mutation through a const Span; either change their return types to "T const*" to enforce element immutability on const Spans, or add a clear comment on the Span class explaining the std::span-like const model (that element constness is encoded in T, not in Span) so users aren’t surprised; also add a short comment for size_address() describing why exposing &s is required/safe and its intended use. Ensure you update the signatures or documentation adjacent to the methods data(), begin(), end(), and size_address() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyphare/pyphare/pharein/simulation.py`:
- Around line 535-537: The bug is that check_restart_options currently returns
an empty dict when no restart_options were supplied, causing populateDict (in
__init__.py) to treat it as present and write default restart keys and call
restarts.validate(sim) with missing "mode"; fix by making check_restart_options
(the code that sets restart_options in simulation.__init__ / function named
check_restart_options) return None when restart_options was not provided in
kwargs (i.e., distinguish absent vs empty), or alternatively change the guard in
populateDict from if simulation.restart_options is not None to a truthiness
check if simulation.restart_options so empty dicts don't trigger the restart
block; update references to simulation.restart_options,
validate_restart_options, and restarts.validate accordingly to ensure behavior
matches the intended absent-vs-empty semantics.
In `@src/core/data/field/field_box_span.hpp`:
- Around line 34-42: The operator*() implementation lacks a compile-time guard
for unsupported template dimension values and uses separate constexpr ifs that
could fall through; update the body of operator*() to use if constexpr / else if
constexpr chains for dim == 1, 2, 3 (preserving the existing assignments to row
= Span<value_type>{...} using arr, box, row_size, row_idx, slab_idx) and add a
static_assert like static_assert(dim==1 || dim==2 || dim==3, "field_box_span:
unsupported dim") to produce a clear compile-time error for other dim values so
the function cannot silently fall through.
In `@src/core/data/field/field_box.hpp`:
- Around line 79-95: The loop in operate_on_fields can advance d_slabs past its
end when dst.lcl_box and src.lcl_box have the same total size but different
per-dimension extents; before creating spans/iterators (or at the top of
operate_on_fields) add an assertion or explicit shape check that per-dimension
extents of dst.lcl_box and src.lcl_box are identical (not just total size),
e.g., compare their shape/extent arrays or call a box-equality predicate used
elsewhere, and apply the same per-dimension check in max_of_fields to ensure
both boxes have identical per-dimension sizes before iterating with
d_slabs/s_slabs.
- Around line 157-163: The function template fill_ghost(Field_t& field, auto
const& layout, auto const v) only writes the left ghost region using
NdArrayMask{0, max} (where max = nghosts - 1) and must also write the trailing
ghost cells; update fill_ghost to compute the trailing range (e.g., starting at
totalSize - nghosts) and set field[NdArrayMask{start, totalSize-1}] = v as well,
using layout.nbrGhosts() and layout.totalSize()/or layout.size() (or the
appropriate accessor on layout) to determine indices, or if you intend to only
fill the left side rename the function to fill_left_ghost to match behavior and
update all call sites accordingly (refer to fill_ghost, layout.nbrGhosts, and
NdArrayMask in the diff).
---
Nitpick comments:
In `@src/core/data/field/field_box_span.hpp`:
- Line 26: Replace the integer null constants used to initialize the Span with
C++11 nullptr: locate the declaration Span<value_type> row{0, 0} and change the
initializer elements from 0 to nullptr so the span's pointer members are
initialized with a proper null pointer (e.g., Span<value_type> row{nullptr,
nullptr}); ensure this change is applied wherever the Span<value_type> row
variable or similar Span initializations appear.
- Around line 51-89: Add a short explanatory comment inside the FieldBoxSlab
template describing its dual iterator/range role: that FieldBoxSlab acts as the
outer iterator (providing operator++, operator==/!= and operator* to return
itself) while also acting as an inner range by exposing begin()/end() that
return FieldBoxRows<Array_t>, enabling nested range-for usage; mention the
relevant symbols FieldBoxSlab, begin(), end(), operator++(), operator*(), and
FieldBoxRows to guide future readers.
In `@src/core/data/field/field_box.hpp`:
- Around line 64-69: The memcpy size is hardcoded to sizeof(double) in
operate_on_field_row (and the other overload) which is fragile; replace
sizeof(double) with the span element size by extracting the span value_type from
the dst (or src) parameter (e.g. using T =
std::remove_reference_t<decltype(dst)>::value_type) and use sizeof(T) for the
memcpy byte count; update both occurrences (the memcpy in operate_on_field_row
and the memcpy in the other overload that copies from src_data into dst) so they
compute the copy size from the span element type instead of assuming double,
keeping the Equals<double> constraint but making the code robust to different
span element types.
In `@src/core/utilities/span.hpp`:
- Around line 41-48: The const-qualified accessors data() const, begin() const
and end() const currently return T* which allows mutation through a const Span;
either change their return types to "T const*" to enforce element immutability
on const Spans, or add a clear comment on the Span class explaining the
std::span-like const model (that element constness is encoded in T, not in Span)
so users aren’t surprised; also add a short comment for size_address()
describing why exposing &s is required/safe and its intended use. Ensure you
update the signatures or documentation adjacent to the methods data(), begin(),
end(), and size_address() accordingly.
| auto& operator*() | ||
| { | ||
| if constexpr (dim == 1) | ||
| return (row = Span<value_type>{&arr(box.lower[0]), row_size}); | ||
| if constexpr (dim == 2) | ||
| return (row = Span<value_type>{&arr(row_idx, box.lower[1]), row_size}); | ||
| if constexpr (dim == 3) | ||
| return (row = Span<value_type>{&arr(slab_idx, row_idx, box.lower[2]), row_size}); | ||
| } |
There was a problem hiding this comment.
Missing fallback for unsupported dimensions in operator*.
If dim is anything other than 1, 2, or 3, the function silently falls through with no return — undefined behavior. A static_assert would catch this at compile time. Also, chaining else if constexpr is slightly more expressive.
Proposed fix
auto& operator*()
{
if constexpr (dim == 1)
return (row = Span<value_type>{&arr(box.lower[0]), row_size});
- if constexpr (dim == 2)
+ else if constexpr (dim == 2)
return (row = Span<value_type>{&arr(row_idx, box.lower[1]), row_size});
- if constexpr (dim == 3)
+ else if constexpr (dim == 3)
return (row = Span<value_type>{&arr(slab_idx, row_idx, box.lower[2]), row_size});
+ else
+ static_assert(dim >= 1 && dim <= 3, "FieldBoxRows: unsupported dimension");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/data/field/field_box_span.hpp` around lines 34 - 42, The operator*()
implementation lacks a compile-time guard for unsupported template dimension
values and uses separate constexpr ifs that could fall through; update the body
of operator*() to use if constexpr / else if constexpr chains for dim == 1, 2, 3
(preserving the existing assignments to row = Span<value_type>{...} using arr,
box, row_size, row_idx, slab_idx) and add a static_assert like
static_assert(dim==1 || dim==2 || dim==3, "field_box_span: unsupported dim") to
produce a clear compile-time error for other dim values so the function cannot
silently fall through.
There was a problem hiding this comment.
no return == compile error
There was a problem hiding this comment.
@PhilipDeegan, you're absolutely right—I apologize for the noise. I should have recognized this pattern from your earlier work in point.hpp, where you use the same deliberate compile-error approach for unsupported types. The absence of a return path for unsupported dimensions ensures a clear compilation failure if anyone tries to instantiate the template with dim outside 1, 2, or 3.
For clarity to future readers, you might consider adding a brief comment like // else no return cause not yet handled (as in point.hpp), but that's entirely optional.
| template<typename Field_t> | ||
| void fill_ghost(Field_t& field, auto const& layout, auto const v) | ||
| { | ||
| std::size_t const nghosts = layout.nbrGhosts(); | ||
| std::size_t const max = nghosts - 1; | ||
| field[NdArrayMask{0, max}] = v; | ||
| } |
There was a problem hiding this comment.
fill_ghost only fills the left ghost region; right ghost is left untouched.
NdArrayMask{0, max} covers indices [0, nghosts−1] — the leading ghost cells. For a field laid out as [ghost | physical | ghost], the trailing ghost region [totalSize−nghosts, totalSize−1] is never written. The function name implies a complete ghost fill.
Either fill both sides or rename to fill_left_ghost to accurately describe the operation:
💡 Proposed fix
template<typename Field_t>
void fill_ghost(Field_t& field, auto const& layout, auto const v)
{
std::size_t const nghosts = layout.nbrGhosts();
std::size_t const max = nghosts - 1;
field[NdArrayMask{0, max}] = v;
+ auto const total = field.size(); // or equivalent accessor
+ field[NdArrayMask{total - nghosts, total - 1}] = v;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/data/field/field_box.hpp` around lines 157 - 163, The function
template fill_ghost(Field_t& field, auto const& layout, auto const v) only
writes the left ghost region using NdArrayMask{0, max} (where max = nghosts - 1)
and must also write the trailing ghost cells; update fill_ghost to compute the
trailing range (e.g., starting at totalSize - nghosts) and set
field[NdArrayMask{start, totalSize-1}] = v as well, using layout.nbrGhosts() and
layout.totalSize()/or layout.size() (or the appropriate accessor on layout) to
determine indices, or if you intend to only fill the left side rename the
function to fill_left_ghost to match behavior and update all call sites
accordingly (refer to fill_ghost, layout.nbrGhosts, and NdArrayMask in the
diff).
There was a problem hiding this comment.
the NdArrayMask is more complicated than you think
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for NdArrayMask definition and implementation
ast-grep --pattern 'struct NdArrayMask {
$$$
}'
# Also search for class definition
ast-grep --pattern 'class NdArrayMask {
$$$
}'
# Search for any NdArrayMask usage and documentation
rg -nC5 'NdArrayMask' --type=cpp --type=hpp
Unrecoverable error: Failed to clone repository
@PhilipDeegan, you're absolutely right—I apologize for the incorrect analysis. I misunderstood how NdArrayMask operates and made incorrect assumptions about it only filling a 1D range. Thank you for the correction!
✏️ Learnings added
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1153
File: src/core/data/field/field_box.hpp:157-163
Timestamp: 2026-02-18T15:16:04.197Z
Learning: In the PHARE codebase, `NdArrayMask` has more sophisticated behavior than a simple 1D index range. When used with multi-dimensional fields, `NdArrayMask{0, max}` correctly handles ghost region filling across all dimensions, not just a left-side 1D range.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-10-09T08:32:15.667Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 784
File: tests/simulator/test_restarts.py:333-339
Timestamp: 2024-07-26T22:04:34.160Z
Learning: PhilipDeegan has indicated a preference for minimal and efficient code, even after making changes to get a function working. There may be opportunities to remove or optimize parts of the code such as sleep/wait/advances.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 1068
File: src/amr/data/field/coarsening/electric_field_coarsener.hpp:1-2
Timestamp: 2025-09-17T13:35:11.533Z
Learning: PhilipDeegan prefers header guard names that include the full directory path structure, following the pattern PHARE_[PATH_WITH_UNDERSCORES]_HPP. For example, a file at src/amr/data/field/coarsening/electric_field_coarsener.hpp should use PHARE_AMR_DATA_FIELD_COARSENING_ELECTRIC_FIELD_COARSENER_HPP as the header guard.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-10-09T08:32:15.667Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
Learnt from: PhilipDeegan
Repo: PHAREHUB/PHARE PR: 888
File: src/hdf5/detail/h5/h5_file.hpp:78-79
Timestamp: 2024-09-05T17:02:58.784Z
Learning: Avoid commenting on draft PRs for the user PhilipDeegan.
5c1a022 to
22feef9
Compare
|
it's possible we might be able to use the md_span here if we were to switch to C++23 |
then we need to assess that and come back to this PR after? |
we might not be able to switch to md_span just yet, so we'll need this either way, then maybe we can dedupe later |
PhilipDeegan
force-pushed
the
new_box_iter
branch
from
22feef9 to
6bb436d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/core/data/field/field_box_span.hpp (1)
65-65: Preserve explicitdimin internal aliases.
FieldBoxSpans_tandFieldBoxSlab_tcurrently drop the class templatedimand fall back to defaults, which is fragile if a non-defaultdiminstantiation is used.🔧 Proposed fix
- using FieldBoxSpans_t = FieldBoxSpans<Array_t>; + using FieldBoxSpans_t = FieldBoxSpans<Array_t, dim>; ... - using FieldBoxSlab_t = FieldBoxSlab<Array_t>; + using FieldBoxSlab_t = FieldBoxSlab<Array_t, dim>;As per coding guidelines:
**/*.hpp: Review the C++ code, point out issues relative to principles of clean code, expressiveness, and performance.Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/field/field_box_span.hpp` at line 65, The internal type aliases drop the template parameter dim and thus use defaults; update the aliases so they preserve the class template parameter by forwarding dim explicitly — e.g., replace uses like FieldBoxSpans_t = FieldBoxSpans<Array_t> and FieldBoxSlab_t = FieldBoxSlab<Array_t> with aliases that pass dim (FieldBoxSpans<Array_t, dim> and FieldBoxSlab<Array_t, dim>) so non-default dim instantiations behave correctly.
🤖 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/core/data/field/field_box.hpp`:
- Around line 84-96: The precondition only compares dst.lcl_box.size() ==
src.lcl_box.size(), which allows different shapes and causes iterator mismatches
during the nested slab/span lockstep loops using make_field_box_span and
operate_on_span<Operator>; fix by enforcing identical box shapes (not just equal
size) before iterating—compare the extents/shape of dst.lcl_box and src.lcl_box
(or add an equality method on the box type) and assert or throw if they differ,
so the paired iterators from make_field_box_span(dst.lcl_box, dst.field) and
make_field_box_span(src.lcl_box, src.field) will always be in lockstep.
- Around line 117-124: Guard FieldBox<Field_t>::set_from against reading past
vec: before calling operate_on_span(span, vec.data() + seek) verify that
vec.size() - seek >= span.size() (or equivalent) and handle the shortfall (throw
std::out_of_range with a clear message or return/error) to avoid undefined
behavior; keep use of make_field_box_span, lcl_box, field, span,
operate_on_span, vec, and seek as-is and only add the size-check and error
handling and ensure seek is only incremented after a successful read.
In `@src/core/utilities/span.hpp`:
- Around line 39-46: Const overloads of operator[], data(), begin(), and end()
return mutable types due to auto deduction; update the const-qualified methods
(operator[](SIZE) const, data() const, begin() const, end() const) to return
const-qualified types (e.g., value_type const& for operator[] const and
value_type const* for pointer returns) so a const Span<T> cannot provide mutable
access; locate and change the return types in those methods in span.hpp
accordingly, keeping the non-const overloads unchanged.
---
Nitpick comments:
In `@src/core/data/field/field_box_span.hpp`:
- Line 65: The internal type aliases drop the template parameter dim and thus
use defaults; update the aliases so they preserve the class template parameter
by forwarding dim explicitly — e.g., replace uses like FieldBoxSpans_t =
FieldBoxSpans<Array_t> and FieldBoxSlab_t = FieldBoxSlab<Array_t> with aliases
that pass dim (FieldBoxSpans<Array_t, dim> and FieldBoxSlab<Array_t, dim>) so
non-default dim instantiations behave correctly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
res/cmake/test.cmakesrc/core/data/field/field_box.hppsrc/core/data/field/field_box_span.hppsrc/core/data/grid/gridlayout.hppsrc/core/utilities/span.hpptests/core/data/field/CMakeLists.txttests/core/data/field/test_field_box_span.cpp
PhilipDeegan
force-pushed
the
new_box_iter
branch
from
6bb436d to
197bdc3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/data/field/field_box.hpp (2)
131-131: Unnecessary type alias shadows class member.Line 131 redeclares
value_typewhich already exists at class scope (line 22). The class-level alias usesstd::decay_tfor the same effect. Consider removing this local declaration.♻️ Proposed fix
template<typename Field_t> void FieldBox<Field_t>::append_to(std::vector<value_type>& vec) { - using value_type = Field_t::value_type; // reserve vec before use! std::size_t seek = vec.size();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/field/field_box.hpp` at line 131, Remove the redundant local type alias declared as "using value_type = Field_t::value_type;" inside field_box; the class already provides a value_type alias (using std::decay_t) at class scope, so delete this duplicate declaration to avoid shadowing Field_t::value_type and rely on the existing class-level value_type.
99-110: Consider using span-based iteration for consistency.This function uses the old box-iterator pattern while
operate_on_fieldsuses the new span-based iteration. For consistency and potential performance benefits (cache-friendly contiguous access), consider refactoring to use spans. Not blocking since the current implementation is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/field/field_box.hpp` around lines 99 - 110, The function max_of_fields uses iterator-based traversal over dst.lcl_box and src.lcl_box; refactor it to use span-based iteration like operate_on_fields to improve consistency and locality. Replace the iterator loop by obtaining spans (or contiguous views) of dst.lcl_box and src.lcl_box, iterate by index or range-for over those spans, and inside the loop use dst.field(span_idx) and src.field(span_idx) to assign dst_val = std::max(dst_val, src_val). Ensure you preserve the assert(dst.lcl_box.size() == src.lcl_box.size()) and keep semantics identical to the original implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/data/field/field_box.hpp`:
- Line 131: Remove the redundant local type alias declared as "using value_type
= Field_t::value_type;" inside field_box; the class already provides a
value_type alias (using std::decay_t) at class scope, so delete this duplicate
declaration to avoid shadowing Field_t::value_type and rely on the existing
class-level value_type.
- Around line 99-110: The function max_of_fields uses iterator-based traversal
over dst.lcl_box and src.lcl_box; refactor it to use span-based iteration like
operate_on_fields to improve consistency and locality. Replace the iterator loop
by obtaining spans (or contiguous views) of dst.lcl_box and src.lcl_box, iterate
by index or range-for over those spans, and inside the loop use
dst.field(span_idx) and src.field(span_idx) to assign dst_val =
std::max(dst_val, src_val). Ensure you preserve the assert(dst.lcl_box.size() ==
src.lcl_box.size()) and keep semantics identical to the original implementation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/data/field/field_box.hppsrc/core/data/field/field_box_span.hppsrc/core/utilities/span.hpp
PhilipDeegan
force-pushed
the
new_box_iter
branch
from
197bdc3 to
1547695
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/data/field/field_box.hpp (1)
99-110:max_of_fieldsuses direct box iteration instead of span-based.This differs from
operate_on_fieldswhich uses span-based iteration. The direct approach is reasonable here since the element-wise max cannot benefit frommemcpyoptimization. Consider adding a brief comment explaining the choice, or unify the iteration pattern for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/field/field_box.hpp` around lines 99 - 110, The function max_of_fields currently iterates directly over dst.lcl_box/src.lcl_box rather than using the span-based pattern used by operate_on_fields; either add a concise comment above max_of_fields explaining that element-wise std::max cannot leverage memcpy-like span optimizations (hence the deliberate box-iterator approach), or refactor max_of_fields to the same span-based loop for consistency while preserving element-wise std::max semantics (ensure you still index using dst.field(...) and src.field(...) when switching to spans).
🤖 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/core/data/field/field_box.hpp`:
- Around line 65-77: The memcpy specialization of operate_on_span should only be
enabled for trivially copyable types: add a compile-time constraint to the
requires clause so the overload for Operator==Equals<T> also requires
std::is_trivially_copyable_v<T>; update the template declaration for
operate_on_span (the Equals<T> branch that calls std::memcpy) to include this
additional constraint to prevent accidental use with non-trivially-copyable T
while keeping the generic operate_on_span unchanged.
In `@src/core/utilities/span.hpp`:
- Line 48: The size_address() method returns a non-const pointer to the internal
member s, breaking const-correctness and exposing mutable access; remove
size_address() from span.hpp if it is unused, or if it must remain for
publicinterop, change its contract to return a const pointer (e.g., const auto*)
or provide a clearly documented and justified alternative and mark it
deprecated; update any callers to use the safe size() accessor and ensure
tests/headers reflect the new signature or removal.
---
Nitpick comments:
In `@src/core/data/field/field_box.hpp`:
- Around line 99-110: The function max_of_fields currently iterates directly
over dst.lcl_box/src.lcl_box rather than using the span-based pattern used by
operate_on_fields; either add a concise comment above max_of_fields explaining
that element-wise std::max cannot leverage memcpy-like span optimizations (hence
the deliberate box-iterator approach), or refactor max_of_fields to the same
span-based loop for consistency while preserving element-wise std::max semantics
(ensure you still index using dst.field(...) and src.field(...) when switching
to spans).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/data/field/field_box.hppsrc/core/data/field/field_box_span.hppsrc/core/utilities/span.hpp
PhilipDeegan
force-pushed
the
new_box_iter
branch
from
1547695 to
292685f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/data/field/field_box_span.hpp (1)
55-61: Storing reference toboxrisks dangling if the originalBoxgoes out of scope.
FieldBoxSpansstoresBox<std::uint32_t, dim> const& boxas a member reference. If theBoxpassed to the constructor is a temporary or goes out of scope before iteration completes, this becomes a dangling reference. The chainFieldBoxSpan→FieldBoxSlab→FieldBoxSpansall store references to the same box, so lifetime is tied to the outermost caller.Consider whether storing by value (copying the
Box) would be safer, given thatBoxis likely small (a few integers).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/data/field/field_box_span.hpp` around lines 55 - 61, FieldBoxSpans stores Box<std::uint32_t, dim> const& box which can dangle; change the member to hold Box<std::uint32_t, dim> box (store by value) and update constructors/initializers to accept either Box by value or const Box& and copy/move into the member, then propagate the same change to the related classes/constructors that forward the box (e.g., FieldBoxSpan, FieldBoxSlab) so they also store/carry the Box by value rather than as a reference, ensuring no dangling reference remains for arr, slab_idx, span_idx, span_size, and span.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/data/field/field_box_span.hpp`:
- Around line 55-61: FieldBoxSpans stores Box<std::uint32_t, dim> const& box
which can dangle; change the member to hold Box<std::uint32_t, dim> box (store
by value) and update constructors/initializers to accept either Box by value or
const Box& and copy/move into the member, then propagate the same change to the
related classes/constructors that forward the box (e.g., FieldBoxSpan,
FieldBoxSlab) so they also store/carry the Box by value rather than as a
reference, ensuring no dangling reference remains for arr, slab_idx, span_idx,
span_size, and span.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/data/field/field_box.hppsrc/core/data/field/field_box_span.hppsrc/core/utilities/span.hpp
3 participants
it's possible we should update field equations to pass the values per index, rather than do the index lookup per operation