Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions src/FoundationClasses/TKMath/GTests/math_Matrix_Test.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -594,3 +594,63 @@ TEST(MathMatrixTest, InPlaceMatrixMultiplication)
EXPECT_NEAR(aMatrixACopy(2, 1), aExpectedAB(2, 1), Precision::Confusion());
EXPECT_NEAR(aMatrixACopy(2, 2), aExpectedAB(2, 2), Precision::Confusion());
}

// Tests for Move Semantics
TEST(MathMatrixTest, MoveSemantics)
{
// --- Move Constructor ---

// Large matrix (heap allocated)
Standard_Integer aRows = 10;
Standard_Integer aCols = 10;
math_Matrix aMat1(1, aRows, 1, aCols);
aMat1.Init(1.0);
aMat1(1, 1) = 2.0;

// Move aMat1 to aMat2
math_Matrix aMat2(std::move(aMat1));

EXPECT_EQ(aMat2.RowNumber(), aRows);
EXPECT_EQ(aMat2.ColNumber(), aCols);
EXPECT_EQ(aMat2(1, 1), 2.0);

// Verify source state (should be empty after move)
EXPECT_EQ(aMat1.RowNumber(), 0);

// Small matrix (buffer allocated)
Standard_Integer aSmallRows = 4;
Standard_Integer aSmallCols = 4;
math_Matrix aSmallMat1(1, aSmallRows, 1, aSmallCols);
aSmallMat1.Init(1.0);

// Move aSmallMat1 to aSmallMat2 (should copy because of buffer)
math_Matrix aSmallMat2(std::move(aSmallMat1));

EXPECT_EQ(aSmallMat2.RowNumber(), aSmallRows);
EXPECT_EQ(aSmallMat2(1, 1), 1.0);

// Source remains valid for buffer-based matrix
EXPECT_EQ(aSmallMat1.RowNumber(), aSmallRows);
EXPECT_EQ(aSmallMat1(1, 1), 1.0);

// --- Move Assignment ---

// Large matrix move assignment
math_Matrix aMatAssign1(1, aRows, 1, aCols);
aMatAssign1.Init(5.0);

math_Matrix aMatAssign2(1, aRows, 1, aCols);
aMatAssign2.Init(0.0);

aMatAssign2 = std::move(aMatAssign1);

EXPECT_EQ(aMatAssign2(1, 1), 5.0);
EXPECT_EQ(aMatAssign1.RowNumber(), 0);

// Assignment dimension mismatch
math_Matrix aMatMismatch1(1, 10, 1, 10);
math_Matrix aMatMismatch2(1, 20, 1, 20);

// Should fall back to copy logic which calls Initialized -> raises DimensionError
EXPECT_THROW(aMatMismatch2 = std::move(aMatMismatch1), Standard_DimensionError);
}
71 changes: 70 additions & 1 deletion src/FoundationClasses/TKMath/GTests/math_Vector_Test.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -638,4 +638,73 @@ TEST(MathVectorTest, EdgeCases)
EXPECT_EQ(aNegVec.Upper(), 1);
EXPECT_EQ(aNegVec.Max(), 1);
EXPECT_EQ(aNegVec.Min(), -2);
}
}

// Tests for Move Semantics
TEST(MathVectorTest, MoveSemantics)
{
// --- Move Constructor ---

// Large vector (heap allocated)
Standard_Integer aLen = 100;
math_Vector aVec1(1, aLen);
for (Standard_Integer i = 1; i <= aLen; ++i)
{
aVec1(i) = static_cast<Standard_Real>(i);
}

// Move aVec1 to aVec2
math_Vector aVec2(std::move(aVec1));

EXPECT_EQ(aVec2.Length(), aLen);
EXPECT_EQ(aVec2(1), 1.0);
EXPECT_EQ(aVec2(aLen), static_cast<Standard_Real>(aLen));

// Verify source state (length should be 0 after move for NCollection_Array1)
// Note: calling Length() is safe as it just returns size.
EXPECT_EQ(aVec1.Length(), 0);

// Small vector (buffer allocated)
Standard_Integer aSmallLen = 10;
math_Vector aSmallVec1(1, aSmallLen);
for (Standard_Integer i = 1; i <= aSmallLen; ++i)
{
aSmallVec1(i) = static_cast<Standard_Real>(i);
}

// Move aSmallVec1 to aSmallVec2 (should copy because of buffer)
math_Vector aSmallVec2(std::move(aSmallVec1));

EXPECT_EQ(aSmallVec2.Length(), aSmallLen);
EXPECT_EQ(aSmallVec2(1), 1.0);

// Source remains valid for buffer-based vector
EXPECT_EQ(aSmallVec1.Length(), aSmallLen);
EXPECT_EQ(aSmallVec1(1), 1.0);

// --- Move Assignment ---

// Large vector move assignment
math_Vector aVecAssign1(1, aLen);
for (Standard_Integer i = 1; i <= aLen; ++i)
{
aVecAssign1(i) = static_cast<Standard_Real>(i);
}

math_Vector aVecAssign2(1, aLen);
aVecAssign2.Init(0.0);

aVecAssign2 = std::move(aVecAssign1);

EXPECT_EQ(aVecAssign2.Length(), aLen);
EXPECT_EQ(aVecAssign2(1), 1.0);

EXPECT_EQ(aVecAssign1.Length(), 0);

// Assignment dimension mismatch
math_Vector aVecMismatch1(1, 100);
math_Vector aVecMismatch2(1, 200);

// Should fall back to copy logic which calls Initialized -> raises DimensionError
EXPECT_THROW(aVecMismatch2 = std::move(aVecMismatch1), Standard_DimensionError);
}
50 changes: 50 additions & 0 deletions src/FoundationClasses/TKMath/math/math_DoubleTab.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <Standard_Boolean.hxx>

#include <array>
#include <utility>

class math_DoubleTab
{
Expand Down Expand Up @@ -77,9 +78,28 @@ public:
{
}

//! Move constructor
math_DoubleTab(math_DoubleTab&& theOther) noexcept
: myArray(theOther.myArray.IsDeletable()
? std::move(theOther.myArray)
: NCollection_Array2<Standard_Real>(*myBuffer.data(),
theOther.LowerRow(),
theOther.UpperRow(),
theOther.LowerCol(),
theOther.UpperCol()))
{
if (!theOther.myArray.IsEmpty())
{
myArray = theOther.myArray;
}
Comment on lines 97 to 100
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}

//! Copy data to theOther
void Copy(math_DoubleTab& theOther) const { theOther.myArray.Assign(myArray); }

//! Returns true if the internal array is deletable (heap-allocated)
Standard_Boolean IsDeletable() const { return myArray.IsDeletable(); }

//! Set lower row index
void SetLowerRow(const Standard_Integer theLowerRow) { myArray.UpdateLowerRow(theLowerRow); }

Expand Down Expand Up @@ -130,6 +150,36 @@ public:
return Value(theRowIndex, theColIndex);
}

//! Assignment operator
math_DoubleTab& operator=(const math_DoubleTab& theOther)
{
myArray = theOther.myArray;
return *this;
}

//! 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;
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
return *this;
}
Comment on lines +169 to +190
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


//! Destructor
~math_DoubleTab() = default;

Expand Down
2 changes: 1 addition & 1 deletion src/FoundationClasses/TKMath/math/math_Matrix.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ public:
math_Matrix& operator=(const math_Matrix& Other) { return Initialized(Other); }

//! Move assignment operator
inline math_Matrix& operator=(math_Matrix&& Other) noexcept;
inline math_Matrix& operator=(math_Matrix&& Other);

//! Returns the product of 2 matrices.
//! An exception is raised if the dimensions are different.
Expand Down
15 changes: 13 additions & 2 deletions src/FoundationClasses/TKMath/math/math_Matrix.lxx
Original file line number Diff line number Diff line change
Expand Up @@ -701,12 +701,23 @@ inline math_Matrix& math_Matrix::Initialized(const math_Matrix& Other)

//==================================================================================================

inline math_Matrix& math_Matrix::operator=(math_Matrix&& Other) noexcept
inline math_Matrix& math_Matrix::operator=(math_Matrix&& Other)
{
if (this != &Other)
if (this == &Other)
{
return *this;
}

if (Array.IsDeletable() && Other.Array.IsDeletable() && RowNumber() == Other.RowNumber()
&& ColNumber() == Other.ColNumber() && LowerRow() == Other.LowerRow()
&& LowerCol() == Other.LowerCol())
{
Array = std::move(Other.Array);
}
else
{
Initialized(Other);
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
Initialized(Other);
// Copy Other before any move operations to avoid copying from a moved-from object
math_Matrix tempOther(Other);
Initialized(tempOther);

Copilot uses AI. Check for mistakes.
}
return *this;
}

Expand Down
8 changes: 7 additions & 1 deletion src/FoundationClasses/TKMath/math/math_VectorBase.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <math_Matrix.hxx>

#include <array>
#include <utility>

//! This class implements the real vector abstract data type.
//! Vectors can have an arbitrary range which must be defined at
Expand Down Expand Up @@ -94,9 +95,11 @@ public:
void Init(const TheItemType theInitialValue);

//! Constructs a copy for initialization.
//! An exception is raised if the lengths of the vectors are different.
inline math_VectorBase(const math_VectorBase& theOther);

//! Move constructor
inline math_VectorBase(math_VectorBase&& theOther) noexcept;

//! Returns the length of a vector
inline Standard_Integer Length() const { return Array.Length(); }

Expand Down Expand Up @@ -255,6 +258,9 @@ public:

math_VectorBase& operator=(const math_VectorBase& theOther) { return Initialized(theOther); }

//! Move assignment operator
inline math_VectorBase& operator=(math_VectorBase&& theOther);

//! returns the inner product of 2 vectors.
//! An exception is raised if the lengths are not equal.
Standard_NODISCARD inline TheItemType Multiplied(const math_VectorBase& theRight) const;
Expand Down
37 changes: 37 additions & 0 deletions src/FoundationClasses/TKMath/math/math_VectorBase.lxx
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,22 @@ math_VectorBase<TheItemType>::math_VectorBase(const math_VectorBase<TheItemType>
{
}

template <typename TheItemType>
math_VectorBase<TheItemType>::math_VectorBase(math_VectorBase<TheItemType>&& theOther) noexcept
: 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())
{
Array = theOther.Array;
}
Comment on lines 90 to 93
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if (!theOther.Array.IsEmpty())
{
Array = theOther.Array;
}

Copilot uses AI. Check for mistakes.
}

template <typename TheItemType>
void math_VectorBase<TheItemType>::SetLower(const Standard_Integer theLower)
{
Expand Down Expand Up @@ -543,6 +559,27 @@ math_VectorBase<TheItemType>& math_VectorBase<TheItemType>::Initialized(
return *this;
}

template <typename TheItemType>
math_VectorBase<TheItemType>& math_VectorBase<TheItemType>::operator=(
math_VectorBase<TheItemType>&& theOther)
{
if (this == &theOther)
{
return *this;
}

if (Array.IsDeletable() && theOther.Array.IsDeletable() && Lower() == theOther.Lower()
&& Length() == theOther.Length())
{
Array.Move(theOther.Array);
}
else
{
Initialized(theOther);
Copy link

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
Initialized(theOther);
Initialized(static_cast<const math_VectorBase<TheItemType>&>(theOther));

Copilot uses AI. Check for mistakes.
}
return *this;
}

template <typename TheItemType>
void math_VectorBase<TheItemType>::Dump(Standard_OStream& theO) const
{
Expand Down
Loading