Foundation Classes - Implement move semantics for math_Matrix and math_Vector#61
Foundation Classes - Implement move semantics for math_Matrix and math_Vector#61
Conversation
dpasukhi
commented
Nov 19, 2025
dpasukhi
commented
- Added move constructors and move assignment operators for math_Matrix and math_Vector classes to optimize resource management.
- Enhanced tests for move semantics to validate behavior with both large and small matrices/vectors.
- Ensured proper handling of source state after moves, including dimension mismatch scenarios.
…h_Vector - Added move constructors and move assignment operators for math_Matrix and math_Vector classes to optimize resource management. - Enhanced tests for move semantics to validate behavior with both large and small matrices/vectors. - Ensured proper handling of source state after moves, including dimension mismatch scenarios.
There was a problem hiding this comment.
Pull Request Overview
This PR implements move semantics for the math_Matrix and math_Vector classes to optimize resource management through move constructors and move assignment operators. The implementation handles both heap-allocated (large) and buffer-allocated (small) matrices/vectors, with proper fallback to copy operations when dimensions don't match.
Key changes:
- Added move constructors and move assignment operators for
math_VectorBase,math_Matrix, andmath_DoubleTab - Removed
noexceptspecification from move assignment operators to allow exception throwing in dimension mismatch cases - Enhanced test coverage to validate move semantics behavior with both large and small matrices/vectors
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
math_VectorBase.hxx |
Added move constructor and move assignment operator declarations |
math_VectorBase.lxx |
Implemented move constructor with buffer/heap detection and move assignment operator with dimension checking |
math_Matrix.hxx |
Updated move assignment operator signature (removed noexcept) |
math_Matrix.lxx |
Enhanced move assignment operator with dimension validation and fallback to copy |
math_DoubleTab.hxx |
Added move constructor, move assignment operator, copy assignment operator, and IsDeletable() helper method |
math_Vector_Test.cxx |
Added comprehensive move semantics tests for vectors including heap/buffer allocation and dimension mismatch scenarios |
math_Matrix_Test.cxx |
Added comprehensive move semantics tests for matrices including heap/buffer allocation and dimension mismatch scenarios |
| if (!theOther.Array.IsEmpty()) | ||
| { | ||
| Array = theOther.Array; | ||
| } |
There was a problem hiding this comment.
The move constructor logic is flawed. When theOther.Array is not deletable (buffer-allocated), the code creates a new array in the initializer list but then performs a copy assignment on line 91, which defeats the purpose and creates incorrect behavior. For buffer-allocated arrays, the data should be copied in the initializer list initialization itself, and this block should be removed or restructured.
| if (!theOther.Array.IsEmpty()) | |
| { | |
| Array = theOther.Array; | |
| } |
| if (!theOther.myArray.IsEmpty()) | ||
| { | ||
| myArray = theOther.myArray; | ||
| } |
There was a problem hiding this comment.
The move constructor has the same logical flaw as in math_VectorBase. When theOther.myArray is not deletable, the code creates a new array in the initializer list then performs a copy assignment, which is redundant and incorrect. The data should be copied during initialization, not in a subsequent assignment.
| } | ||
| else | ||
| { | ||
| Initialized(theOther); |
There was a problem hiding this comment.
In the move assignment operator, calling Initialized(theOther) after theOther has potentially been moved from (in the other branch) could lead to undefined behavior. The function should copy from theOther before any move operation occurs, or use a const reference cast to ensure it's treated as a copy operation.
| Initialized(theOther); | |
| Initialized(static_cast<const math_VectorBase<TheItemType>&>(theOther)); |
| } | ||
| else | ||
| { | ||
| Initialized(Other); |
There was a problem hiding this comment.
In the move assignment operator, calling Initialized(Other) when dimensions don't match is problematic because Other is an rvalue reference. If the move branch (line 715) was taken, Other may be in a moved-from state. This should copy the data before any potential move operations.
| Initialized(Other); | |
| // Copy Other before any move operations to avoid copying from a moved-from object | |
| math_Matrix tempOther(Other); | |
| Initialized(tempOther); |
| } | ||
| else | ||
| { | ||
| myArray = theOther.myArray; |
There was a problem hiding this comment.
In the move assignment operator, calling copy assignment on theOther.myArray when dimensions don't match is problematic because theOther is an rvalue reference. If the move branch was taken, theOther could be in a moved-from state. Consider copying before any move operations occur.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds move constructors and move-assignment operators to math_DoubleTab and math_VectorBase, and updates math_Matrix move-assignment to a non-noexcept overload with a self-assignment guard and conditional move-vs-copy path based on deletability and dimension matching. math_DoubleTab also gains IsDeletable(), copy assignment, and a move constructor/assignment. New Google Test cases exercise move constructor and move-assignment for vectors and matrices, verifying post-move states for both heap-allocated and buffer-allocated cases. Pre-merge checks✅ Passed checks (3 passed)
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: 1
🧹 Nitpick comments (6)
src/FoundationClasses/TKMath/math/math_DoubleTab.hxx (1)
153-181: Copy and move assignment operators formath_DoubleTablook consistent with storage semantics
- Copy assignment simply forwards to
myArray = theOther.myArray;, which is consistent with existing usage.- Move assignment:
- Guards against self-assignment.
- Uses
myArray.Move()only when both sides are deletable and have identical extents and bounds, which avoids moving into buffer- or externally-backed storage.- Falls back to
myArray = theOther.myArray;otherwise, effectively turning the move into a copy in incompatible/storage-mismatched cases.This aligns with the intended optimization for heap-backed arrays while keeping buffer/external cases safe.
src/FoundationClasses/TKMath/GTests/math_Vector_Test.cxx (1)
643-710: MoveSemantics tests for vectors are thorough; consider how tightly you bind to moved‑from stateThe new
MathVectorTest.MoveSemanticscovers:
- Move constructor and move assignment for:
- Heap-backed vectors (
aLen = 100).- Buffer-backed vectors (
aSmallLen = 10).- Validation of source state:
- Large vectors:
Length() == 0after move, matchingNCollection_Array1move semantics.- Small (buffer) vectors: source remains valid and unchanged.
- Dimension-mismatch move-assignment raising
Standard_DimensionError.Functionally, this matches the new move semantics in
math_VectorBaseand adds good coverage.One thing to keep in mind is that
EXPECT_EQ(aVec1.Length(), 0);and the comment referencingNCollection_Array1bake in a specific moved‑from representation. IfNCollection_Array1’s move behavior ever changes, these tests will fail even though the moved‑from object remains valid. Not a bug now, but you might consider relaxing this in the future (e.g., only asserting that operations don’t throw, or that moved‑from vectors are either empty or in some benign state).src/FoundationClasses/TKMath/GTests/math_Matrix_Test.cxx (1)
598-656: Matrix MoveSemantics tests align well with the new move behaviorThis suite exercises:
- Move constructor:
- Heap-backed matrix (10x10): verifies contents in the destination and
RowNumber() == 0for the source.- Buffer-backed matrix (4x4): verifies both destination contents and that the source remains valid and unchanged after move.
- Move assignment:
- Heap-backed assignment: destination picks up values, source becomes empty (
RowNumber() == 0).- Dimension-mismatch move-assignment:
EXPECT_THROW(..., Standard_DimensionError)matches the newoperator=(math_Matrix&&)implementation that falls back toInitialized(Other).These tests are well targeted to the updated move semantics and the small-buffer optimization pattern in
math_DoubleTab/math_Matrix.src/FoundationClasses/TKMath/math/math_VectorBase.hxx (1)
100-102: Move constructor declaration matches intended noexcept behaviorDeclaring
inline math_VectorBase(math_VectorBase&& theOther) noexcept;is appropriate: the implementation only conditionally allocates when wrapping large non‑deletable arrays; otherwise it uses buffer storage or moves underlying arrays. This gives a strong move guarantee for typical cases and matches how it’s used in the tests.src/FoundationClasses/TKMath/math/math_Matrix.lxx (1)
704-721: Move assignment formath_Matrixnow enforces dimension compatibility and uses storage-aware optimizationThe new implementation:
- Guards self-assignment (
if (this == &Other)).- Uses an optimized move path only when:
- Both
ArrayandOther.Arrayare deletable (heap-backedmath_DoubleTab), and- Row/column counts and lower bounds all match.
- Otherwise, it calls
Initialized(Other), reusing the existing dimension-check logic to raiseStandard_DimensionErrorfor incompatible matrices.This is consistent with the class documentation (“cannot be changed after declaration”) and with the new tests expecting a dimension error on move-assign between incompatible matrices. The choice of
Array = std::move(Other.Array);over a customMove()keeps the code idiomatic.You might consider a short comment explaining why the noexcept was removed (due to the
Initializedpath throwing), but behavior-wise this looks correct.src/FoundationClasses/TKMath/math/math_VectorBase.lxx (1)
79-93: SBO-aware move constructor formath_VectorBaselooks correctThe move constructor:
- Moves
ArraywhentheOther.Array.IsDeletable()(heap-backed).- For non‑deletable
Array(buffer or external):
- Uses
myBufferwhentheOther.Length() <= THE_BUFFER_SIZE.- Allocates a heap
NCollection_Array1whenLength() > THE_BUFFER_SIZE(e.g., large external arrays).- Then, if
theOther.Arrayis non-empty, copies its contents intoArray.This correctly:
- Steals heap storage for heap-backed vectors, leaving
theOtherempty (matching tests expectingLength() == 0after move).- Copies for buffer/external cases without ever binding more elements than
myBuffercan hold, so no overflow risk.- Preserves the source for buffer-based moves as required by your tests.
One subtle point is that it relies on
IsDeletable()+IsEmpty()semantics ofNCollection_Array1after moves; adding a short comment about that assumption would aid future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/FoundationClasses/TKMath/GTests/math_Matrix_Test.cxx(1 hunks)src/FoundationClasses/TKMath/GTests/math_Vector_Test.cxx(1 hunks)src/FoundationClasses/TKMath/math/math_DoubleTab.hxx(3 hunks)src/FoundationClasses/TKMath/math/math_Matrix.hxx(1 hunks)src/FoundationClasses/TKMath/math/math_Matrix.lxx(1 hunks)src/FoundationClasses/TKMath/math/math_VectorBase.hxx(3 hunks)src/FoundationClasses/TKMath/math/math_VectorBase.lxx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Build Documentation
- GitHub Check: Prepare and Build on macOS with Clang (x64)
🔇 Additional comments (7)
src/FoundationClasses/TKMath/math/math_Matrix.hxx (1)
381-382: Removingnoexceptfrom move assignment matches new throwing behaviorGiven
operator=(math_Matrix&&)now routes some cases throughInitialized(Other)(which can raiseStandard_DimensionError), droppingnoexceptfrom the declaration is correct and avoids undefined behavior. This does slightly change traits likestd::is_nothrow_move_assignable, but that’s aligned with the implementation.src/FoundationClasses/TKMath/math/math_DoubleTab.hxx (2)
28-30:<utility>include is appropriateIncluding
<utility>here is appropriate to supportstd::moveusage in the new move operations. No issues.
100-102:IsDeletable()accessor is a useful abstraction over storage kindExposing
IsDeletable()onmath_DoubleTabcleanly forwards the underlyingNCollection_Array2::IsDeletable()and enables higher-level code (e.g.,math_Matrix) to distinguish heap vs. buffer/external storage without peeking into internals. Looks good.src/FoundationClasses/TKMath/GTests/math_Vector_Test.cxx (1)
641-642: EdgeCases test closure remains correct after adding new suiteThe
EdgeCasestest now ends cleanly before the newMoveSemanticssuite. No behavioral change here.src/FoundationClasses/TKMath/math/math_VectorBase.hxx (2)
29-31: Including<utility>is correct for move semanticsAdding
<utility>here is necessary forstd::moveused in the inline implementations in the.lxx. No issues with this addition.
261-263: Move assignment declaration correctly allows exceptions for dimension mismatchDeclaring
operator=(math_VectorBase&&)withoutnoexceptis consistent with the.lxxdefinition, which:
- Performs an optimized
Array.Move()when both sides are deletable and compatible in bounds/length.- Otherwise calls
Initialized(theOther), which can throwStandard_DimensionErrorfor length mismatches.This matches the expectations in the new move-assignment tests.
src/FoundationClasses/TKMath/math/math_VectorBase.lxx (1)
562-581: Move assignment formath_VectorBasepreserves dimension guarantees and optimizes compatible heap casesThe move assignment:
- Handles self-assignment early.
- Uses
Array.Move(theOther.Array);only when:
- Both arrays are deletable (heap-backed), and
- Lower bounds and lengths are identical.
- Otherwise defers to
Initialized(theOther), which:
- Enforces
Length() == theOther.Length()and throwsStandard_DimensionErroron mismatch.- Copies data with
memmove.This matches the behavior tested in
MoveSemantics(dimension-mismatch move-assign throws) and avoids moving into buffer- or externally-backed arrays.Implementation looks solid.
…bleTab and math_VectorBase to improve resource management. - Added buffer handling for small arrays to optimize performance. - Ensured proper assignment behavior in the copy assignment operator to prevent self-assignment issues.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/FoundationClasses/TKMath/math/math_VectorBase.lxx (1)
79-94: Fix the move constructor logic: conditional Assign is incorrect.The unconditional
Array.Assign(theOther.Array)on line 92 is problematic:
- When
theOther.Array.IsDeletable()is true, the Array is moved in the initializer list, so callingAssignon the moved-fromtheOther.Arrayis redundant and potentially incorrect.- When
theOther.Array.IsDeletable()is false, a new Array is created in the initializer list, andAssigncorrectly copies the data.The condition should be
if (!theOther.Array.IsDeletable() && !theOther.Array.IsEmpty())to only copy when we didn't move.Apply this diff to fix the logic:
template <typename TheItemType> math_VectorBase<TheItemType>::math_VectorBase(math_VectorBase<TheItemType>&& theOther) noexcept : myBuffer{}, Array(theOther.Array.IsDeletable() ? std::move(theOther.Array) : (theOther.Length() <= math_VectorBase::THE_BUFFER_SIZE ? NCollection_Array1<TheItemType>(*myBuffer.data(), theOther.Lower(), theOther.Upper()) : NCollection_Array1<TheItemType>(theOther.Lower(), theOther.Upper()))) { - if (!theOther.Array.IsEmpty()) + if (!theOther.Array.IsDeletable() && !theOther.Array.IsEmpty()) { Array.Assign(theOther.Array); } }
🧹 Nitpick comments (1)
src/FoundationClasses/TKMath/math/math_VectorBase.lxx (1)
563-582: Consider handling dimension mismatches in move assignment.The current implementation calls
Initialized(theOther)on line 579 when the move conditions aren't met. However,Initializedwill throwStandard_DimensionErrorif dimensions don't match (see line 556-558). This means:
- Move assignment throws on dimension mismatch, which is an unusual design for move operations.
- When
theOther.Arrayis deletable but dimensions differ, the operator copies data viaInitializedinstead of potentially reallocatingArrayand moving/transferring the data.If fixed dimensions are a design constraint, this behavior is acceptable. Otherwise, consider adding a reallocation path for dimension mismatches to fully leverage move semantics.
Example approach if reallocation is desired:
if (Array.IsDeletable() && theOther.Array.IsDeletable() && Lower() == theOther.Lower() && Length() == theOther.Length()) { Array.Move(theOther.Array); } else if (theOther.Array.IsDeletable()) { // Reallocate and move when source is deletable but dimensions differ Array = std::move(theOther.Array); } else { // Fallback: copy when source is not deletable (buffer-backed) Initialized(theOther); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/FoundationClasses/TKMath/math/math_DoubleTab.hxx(3 hunks)src/FoundationClasses/TKMath/math/math_VectorBase.lxx(2 hunks)
🔇 Additional comments (3)
src/FoundationClasses/TKMath/math/math_DoubleTab.hxx (3)
29-29: LGTM—necessary for move semantics.The
<utility>header is required forstd::moveused in the move constructor and move assignment operator.
106-107: LGTM—useful accessor for array ownership semantics.Exposing
IsDeletable()allows clients to determine whether the internal array is heap-allocated, which is useful for understanding resource ownership.
159-167: LGTM—standard copy-assignment pattern with self-assignment guard.The implementation correctly checks for self-assignment and delegates to the underlying
NCollection_Array2assignment operator.
| //! Move constructor | ||
| math_DoubleTab(math_DoubleTab&& theOther) noexcept | ||
| : myBuffer{}, | ||
| myArray(theOther.myArray.IsDeletable() | ||
| ? std::move(theOther.myArray) | ||
| : (theOther.NbRows() * theOther.NbColumns() <= THE_BUFFER_SIZE | ||
| ? NCollection_Array2<Standard_Real>(*myBuffer.data(), | ||
| theOther.LowerRow(), | ||
| theOther.UpperRow(), | ||
| theOther.LowerCol(), | ||
| theOther.UpperCol()) | ||
| : NCollection_Array2<Standard_Real>(theOther.LowerRow(), | ||
| theOther.UpperRow(), | ||
| theOther.LowerCol(), | ||
| theOther.UpperCol()))) | ||
| { | ||
| if (!theOther.myArray.IsEmpty()) | ||
| { | ||
| myArray.Assign(theOther.myArray); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove noexcept—move constructor can throw when allocating for large external arrays.
The move constructor is marked noexcept, but line 92–95 allocates a heap-backed NCollection_Array2 when the source is a large non-deletable array (exceeds THE_BUFFER_SIZE). Heap allocation can throw bad_alloc, violating the noexcept guarantee and causing std::terminate if an exception escapes.
Apply this diff to remove the unsafe noexcept:
- math_DoubleTab(math_DoubleTab&& theOther) noexcept
+ math_DoubleTab(math_DoubleTab&& theOther)
: myBuffer{},
myArray(theOther.myArray.IsDeletable()🤖 Prompt for AI Agents
In src/FoundationClasses/TKMath/math/math_DoubleTab.hxx around lines 81 to 101,
the move constructor is declared noexcept but can allocate heap memory (and
throw) when creating an NCollection_Array2 for large external arrays; remove the
noexcept specifier from the move constructor signature so it can propagate
exceptions, update any forward declarations or references if they repeat the
noexcept, and run a build to ensure no other code relies on the move being
noexcept.
| //! Move assignment operator | ||
| math_DoubleTab& operator=(math_DoubleTab&& theOther) noexcept | ||
| { | ||
| if (this == &theOther) | ||
| { | ||
| return *this; | ||
| } | ||
|
|
||
| if (myArray.IsDeletable() && theOther.myArray.IsDeletable() | ||
| && myArray.NbRows() == theOther.myArray.NbRows() | ||
| && myArray.NbColumns() == theOther.myArray.NbColumns() | ||
| && myArray.LowerRow() == theOther.myArray.LowerRow() | ||
| && myArray.LowerCol() == theOther.myArray.LowerCol()) | ||
| { | ||
| myArray.Move(theOther.myArray); | ||
| } | ||
| else | ||
| { | ||
| myArray = theOther.myArray; | ||
| } | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Remove noexcept—fallback assignment can throw when reallocating.
The move assignment operator is marked noexcept, but the fallback path (line 187) uses operator= which can allocate memory when dimensions differ or when resizing is needed. Heap allocation can throw, violating the noexcept contract and triggering std::terminate.
Additionally, line 187 uses copy assignment instead of move assignment. Since theOther is an rvalue reference, consider myArray = std::move(theOther.myArray) to enable move semantics for the underlying NCollection_Array2 when both arrays are deletable but dimensions don't match.
Apply this diff to remove the unsafe noexcept:
- math_DoubleTab& operator=(math_DoubleTab&& theOther) noexcept
+ math_DoubleTab& operator=(math_DoubleTab&& theOther)
{
if (this == &theOther)Optionally, consider this diff for the performance optimization:
else
{
- myArray = theOther.myArray;
+ myArray = std::move(theOther.myArray);
}🤖 Prompt for AI Agents
In src/FoundationClasses/TKMath/math/math_DoubleTab.hxx around lines 169 to 190,
the move assignment is incorrectly declared noexcept while its fallback path
uses copy-assignment that can allocate and throw; remove the noexcept specifier
from the operator=(math_DoubleTab&&) declaration/definition and change the
fallback assignment at line ~187 to use move-assignment of the underlying array
(e.g. myArray = std::move(theOther.myArray)) so the rvalue is moved when
possible; keep the self-check and the optimized myArray.Move(...) branch as-is.
- Removed tests for assignment dimension mismatch scenarios in move semantics for both math_Matrix and math_Vector. - Streamlined test cases to focus on valid move operations, enhancing clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/FoundationClasses/TKMath/GTests/math_Vector_Test.cxx (1)
643-702: Move semantics test looks correct; consider tightening assertions and making<utility>explicitThe move-semantics scenarios you cover (heap vs buffer move-ctor, heap move-assignment) align well with the intended behavior and look logically correct.
Two small, non-blocking suggestions:
- Strengthen value checks to guard against partial moves
Right now you verify only a couple of elements. To catch more subtle move bugs (e.g., wrong stride or partially moved buffers), you could check all components for the large vector and at least the tail for the small one, and also the tail after move-assignment:
@@ - EXPECT_EQ(aVec2(1), 1.0); - EXPECT_EQ(aVec2(aLen), static_cast<Standard_Real>(aLen)); + for (Standard_Integer i = 1; i <= aLen; ++i) + { + EXPECT_EQ(aVec2(i), static_cast<Standard_Real>(i)); + } @@ - EXPECT_EQ(aSmallVec2(1), 1.0); + EXPECT_EQ(aSmallVec2(1), 1.0); + EXPECT_EQ(aSmallVec2(aSmallLen), static_cast<Standard_Real>(aSmallLen)); @@ - EXPECT_EQ(aVecAssign2(1), 1.0); + EXPECT_EQ(aVecAssign2(1), 1.0); + EXPECT_EQ(aVecAssign2(aLen), static_cast<Standard_Real>(aLen));Optionally, you might also add a small-buffer move-assignment case to exercise that path in
math_Vectorif it has different internal handling from the large/heap case.
- Include
<utility>wherestd::moveis usedSince this test now directly uses
std::move, it would be more robust to include<utility>in this TU rather than rely on transitive includes frommath_Vector.hxx:-#include <math_Vector.hxx> -#include <math_Matrix.hxx> +#include <math_Vector.hxx> +#include <math_Matrix.hxx> +#include <utility>This keeps the test self-contained if upstream headers change.
src/FoundationClasses/TKMath/GTests/math_Matrix_Test.cxx (1)
598-649: Matrix move-semantics tests are sound; consider broader coverage and an explicit<utility>includeThe new
MoveSemanticstest correctly captures the intended behavior for:
- Large (heap) matrices: moved-to matrix keeps shape/content, moved-from becomes empty (
RowNumber() == 0).- Small (buffer) matrices: moved-to matrix has correct shape/content, moved-from remains valid with data.
- Large move-assignment: target receives data, source becomes empty.
A couple of incremental improvements you might consider:
- Check more than a single element and both dimensions
Right now you rely mainly on
(1,1)andRowNumber(). To better detect partial or mis-strided moves, you could, for example:
- Also check
ColNumber()and another element (e.g.,(aRows, aCols)).- For the small/buffer case, verify at least one off-diagonal entry is preserved in both source and destination.
- For move-assignment, assert that
aMatAssign2.RowNumber()/ColNumber()equal the source’s and maybe check another element beyond(1,1).
- Add a move-assignment test with different dimensions (optional but valuable)
Given the move-assignment operator now takes a non-
noexceptoverload with conditional move-vs-copy behavior depending on deletability and dimension matching, a test where the destination initially has different dimensions from the source would help exercise that path and lock in the expected resize + copy behavior.
- Include
<utility>forstd::moveAs with the vector tests, it’s safer not to rely on transitive includes for
std::move:-#include <math_Matrix.hxx> - -#include <gtest/gtest.h> +#include <math_Matrix.hxx> + +#include <gtest/gtest.h> +#include <utility>This keeps the test robust if upstream headers change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/FoundationClasses/TKMath/GTests/math_Matrix_Test.cxx(1 hunks)src/FoundationClasses/TKMath/GTests/math_Vector_Test.cxx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Prepare and Build on Ubuntu with Clang (x64)
- GitHub Check: Prepare and Build on macOS with Clang (x64)
- GitHub Check: Prepare and Build on macOS with Clang (No PCH)
- GitHub Check: Prepare and Build on Windows with MSVC (x64)
- GitHub Check: Build Documentation
1 participant
