[LinearAlgebra] CompressedRowSparseMatrix family: Remove unused/dead code - #6271
Open
fredroy wants to merge 12 commits into
Open
[LinearAlgebra] CompressedRowSparseMatrix family: Remove unused/dead code#6271fredroy wants to merge 12 commits into
fredroy wants to merge 12 commits into
Conversation
block() short-circuits on "is j the first or the last column of this row?" without first checking that the row range is non-empty. A registered row may hold no block at all -- fullRows() and fullDiagonal() both create such rows -- and then colsIndex[rowRange.first] reads the next row's first entry while colsIndex[rowRange.second - 1] reads the previous row's last one, which is index -1 for the first row. Reading any empty row therefore returned another row's value, or read out of bounds. On a 6x6 matrix holding only (4,4) = 7, block(0,4) returned 7 and block(0,0) returned uninitialised memory. The same row lookup and column lookup are open-coded at a dozen sites, so extract them into findRow() and findColInRange() and use them here. Both helpers fold in the guards the copies were missing: the empty-range check, and the nBlockRow == 0 / nBlockCol == 0 division guard. Fixes CompressedRowSparseMatrixGeneric.BlockOnEmptyRowAfterFullRows, CompressedRowSparseMatrixGeneric.BlockOnEmptyRowDoesNotBorrowNeighbourValue and CompressedRowSparseMatrixMechanical.ElementAfterFullRows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 2b234b6ec19dc962d0a24590c59e390869c2c27e)
clearRowBlock() called rowIndex.back() and rowIndex.front() as fast paths before checking that rowIndex was non-empty, so clearing a row of a matrix with no registered row dereferenced past a null pointer and segfaulted. findRow() already reports an empty matrix as "row not found", so use it here instead of the open-coded lookup. Fixes CompressedRowSparseMatrixGenericDeathTest.ClearRowBlockOnEmptyMatrix, CompressedRowSparseMatrixGenericDeathTest.ClearRowBlockOnDefaultConstructedMatrix and CompressedRowSparseMatrixConstraintDeathTest.ClearRowBlockOnEmptyMatrix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 14d101ebcb2fa13c352e2ec298e54c8137c0b40b)
clearRowColBlock() computed foundRowId but never acted on it. When the row lookup failed, rowId kept whatever value the failed binary search left behind, and that value was then used to build rowRange -- reading out of bounds -- and passed straight to deleteRow(). On a matrix where row i is absent but column i exists, this dropped a completely unrelated row: clearing row/column 0 of a matrix holding (1,0), (1,1) and (2,2) deleted row 1, losing (1,1). An absent row is normal in a sparse matrix and column i may still hold blocks in other rows, so it is not an error either. Reserve the diagnostic for an index that is genuinely outside the matrix, delete the row only when findRow() locates it, and always clear the column. clearColBlock() carried its own copy of the unguarded column lookup, so route it through findColInRange() as well since clearRowColBlock() delegates to it. Fixes CompressedRowSparseMatrixGeneric.ClearRowColBlockWithAbsentRow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 3275851fb0e1c693bb1f4828221867d26aa44ece)
Two defects in the symmetric half of clearRowCol(). First, having read the block column j out of colsIndex, it built the symmetric block's range as rowBegin[j] .. rowBegin[j + 1]. rowBegin is indexed by the position of a row inside rowIndex, not by the row number; the two coincide only when every row is present. On a sparse matrix this searched the wrong row -- reading out of bounds when j exceeded the number of registered rows -- and so failed to clear entries that are in column i. Clearing row/column 2 of an 8x8 matrix holding rows 2..5 searched row 5's range for the symmetric block and left (3,2) untouched. Second, when the symmetric block (j,i) was not found, the local pointer was never reset, so the second loop zeroed a column of block (i,j) instead -- entries that lie in neither row i nor column i. Only observable for NL > 1; with 3x3 blocks, clearing row/column 0 wiped (1,3) and (2,3). Look row j up with findRow() before taking its range, locate the symmetric block with findColInRange(), and skip the second clear entirely when it does not exist. Handle the diagonal block explicitly, since it is its own symmetric counterpart. This also drops an unguarded division by nBlockRow. Fixes CompressedRowSparseMatrixMechanical.ClearRowColMissesSymmetricBlockWhenRowsAreSparse and CompressedRowSparseMatrixMechanical.ClearRowColAsymmetricPatternMat3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 230d96cf49e3d0d895468d481e4f728cd20ab517)
filterValues() only pushed a destination row when that row had produced at least one value, so the keepEmptyRows flag threaded through all eight copy*() wrappers did nothing. The compensating pop_back() meant to undo the push could never fire either: rows are only pushed once oldVid != vid, so rowBegin.back() is always strictly less than vid at that point. Push the row when the caller asked to keep empty ones, and drop the dead pop_back(). The flag concerns destination rows emptied by the filter itself, not source rows that were never registered: filterValues() only iterates srcMatrix.rowIndex. No caller in the tree passes keepEmptyRows; every call uses the default false. Fixes CompressedRowSparseMatrixMechanical.CopyNonZerosKeepEmptyRows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RowConstIterator's defaulted default constructor left m_internal and m_matrix without initialisers. A default-constructed iterator therefore held an indeterminate row position and a dangling matrix pointer, and its own isInvalid() reported false -- so it read as a valid iterator onto row 0 of an unspecified matrix. Give both members a default member initialiser so the default-constructed state is the invalid one the class already knows how to describe. Fixes CompressedRowSparseMatrixConstraint.DefaultConstructedRowIteratorIsInvalid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…arded helpers The "find row i" and "find column j inside a row" idioms were open-coded at a dozen more sites. Each copy carried the same two hazards the previous commits fixed at individual call sites: an unguarded division by nBlockRow / nBlockCol, undefined on a default-constructed matrix, and the first/last-column fast paths dereferencing colsIndex without checking the row range for emptiness. The division silently yields 0 on ARM, which is why the accompanying guard tests pass there, but integer division by zero raises SIGFPE on x86_64. Confirmed with UBSan at Mechanical.h:343 (clearRow), :748 (blockGet) and :841 (bRowBegin) before this change, clean after. Converted: clearRow, blockGet, blockGetW, blockCreate, bRowBegin, bRowEnd and bRowRange in the mechanical matrix; both wblock() overloads -- including the hinted one, whose divisions had no guard at all -- and getMaxColIndex in the generic one. The ordered-insertion append path now treats an empty range on the last row as "append here" rather than reading colsIndex[-1]. No behaviour change on well-formed input; this is the undefined behaviour left over once the six defects with failing tests were fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
check_matrix() has no callers anywhere in the tree, and would not work if it had any: - the non-static overload takes &rowBegin[0], &colsIndex[0] and &colsValue[0] without checking the vectors are non-empty; - it walks a_p from 1 to m, where m is rowBSize(), but rowBegin holds rowIndex.size() + 1 entries -- the two only match after fullRows(); - it requires a_p to increase strictly, so any empty row is reported as an error; - the column loop increments i both in the for-header and in the body, so most entries are never checked; - one diagnostic prints a_p[i] where it means a_i[i]; - and success is reported through msg_error. Delete it rather than repair a validator nothing calls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
touchedBlock was declared alongside the CSR arrays and cleared once in compressBtemp(), but never written to and never read -- by this class or by anything else in the tree. Its documented purpose, tracking which blocks were touched since the last compression, was never implemented. It was also the one data member swap() did not exchange, so swapping two matrices left it behind; removing it makes that discrepancy moot. The VecFlag alias it used is kept: it is now unused in-tree, but it is public API on both CRSBlockTraits and the matrix classes, so out-of-tree code may still name it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither helper has ever been called: write()/read() serialise the CSR arrays through the stream operators instead, and nothing outside the class can reach these two since they are protected. readVector() would also have been wrong for most instantiations. It parses every entry through safeStrToInt into an int before pushing it back, so it cannot round-trip a vector of blocks or of any real type -- only the index vectors. Dropping them leaves sofa/type/hardening.h unused in this header, so remove that include too. Verified that no transitive consumer depended on it by rebuilding Sofa.Core, Sofa.Component.Constraint.Projective, Sofa.Component.LinearSolver.Direct, Sofa.Component.LinearSystem, Sofa.Component.StateContainer, Sofa.Component.SolidMechanics.FEM.Elastic and Sofa.Component.Mapping.Linear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RowConstIterator declared
template <class VecDeriv, typename Real>
Real operator*(const VecDeriv& v) const
but Real appears only in the return type, so it can be neither deduced from the
argument nor defaulted, and operator syntax offers no way to supply it
explicitly. No call to it can compile: overload resolution does not even
consider the member, and a *rowIt * v expression fails with "indirection
requires pointer operand".
The functionality it duplicated is available, and actually used, through the
free CompressedRowSparseMatrixVecDerivMult(row, vec), which defaults Real to
VecDeriv::value_type::Real.
Removed rather than repaired: reinstating it would mean giving Real that same
default, which turns a member nothing has ever been able to call into new API.
That is a deliberate addition, not dead-code cleanup, so it is left out of this
series.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fredroy
added
pr: breaking
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
1 participant
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.
Based on
Removing dead code/unreachable functions, or even in-compilable code (but never reached)
d3aac90 check_matrix() — both overloads, no callers, non-functional
289ebfe touchedBlock — declared, cleared once, never read or written
073c5f8 readVector() / writeVector() — protected, never called
d90377f RowConstIterator::operator* — uncallable, Real not deducible
diff : fredroy/sofa@fix_crs_problematic_unittests...crs_remove_deadcode
I guess it is making this PR breaking...
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if