COMP: Use std::unique_ptr in FEMLinearSystemWrapperDenseVNL

[hjmjohnson]

Summary

Replace raw new/delete of matrix and vector holders in FEMLinearSystemWrapperDenseVNL with std::unique_ptr and std::make_unique, following C++ Core Guidelines R.11 and R.20.

• Replace raw new/delete of vnl_matrix and vnl_vector holder objects with std::unique_ptr and std::make_unique
• Use std::swap for all swap methods instead of manual pointer shuffling
• Default the destructor (no manual delete needed)

Test plan

• CI builds pass on all platforms
• FEM module tests pass (ctest -R FEM)

Part of a series splitting out #5993

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR modernizes LinearSystemWrapperDenseVNL by replacing raw new/delete memory management with std::unique_ptr and std::make_unique, following C++ Core Guidelines R.11 and R.20. The changes are correct and well-scoped: the three private holder members (m_Matrices, m_Vectors, m_Solutions) are now std::unique_ptr-managed, the destructor is safely defaulted (relying on unique_ptr RAII), and all swap operations are simplified via std::swap on unique_ptrs.

Key changes:

• MatrixHolder element type upgraded from raw MatrixRepresentation* to std::unique_ptr<MatrixRepresentation>; outer holder wrapped in std::unique_ptr<MatrixHolder>
• m_Vectors and m_Solutions similarly wrapped; all new/delete calls replaced with std::make_unique and .reset()
• SwapMatrices, SwapVectors, and SwapSolutions all consolidated to std::swap on unique_ptrs — note that SwapVectors previously did a deep value copy-swap while the new code does a pointer swap, but observable behavior through the class interface is identical
• Manual destructor (which explicitly looped through all indices and called delete) is replaced by = default, correctly relying on unique_ptr chain destruction
• Minor: m_Vectors/m_Solutions inline types are verbose and repeated, while MatrixHolder has a named alias — a VectorHolder alias would improve symmetry (style suggestion only)

Confidence Score: 5/5

Safe to merge — all changes are semantically equivalent, memory management is correct, and no regressions are introduced.

All findings are P2 style suggestions (a missing VectorHolder type alias). No logic errors, no semantic regressions, and the unique_ptr-based RAII pattern is strictly safer than the old manual new/delete loop. The only behavioural difference in SwapVectors (pointer swap vs. value swap) is equivalent through the class's public interface.

No files require special attention.

Important Files Changed

Filename	Overview
Modules/Numerics/FEM/include/itkFEMLinearSystemWrapperDenseVNL.h	Modernized member types from raw pointers to std::unique_ptr; added #include ; defaulted destructor. Minor style note: VectorHolder alias inconsistency.
Modules/Numerics/FEM/src/itkFEMLinearSystemWrapperDenseVNL.cxx	Replaced all new/delete with std::make_unique/reset, manual pointer swaps with std::swap, and removed the manual destructor body. All changes are semantically correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[LinearSystemWrapperDenseVNL] --> B["m_Matrices\nunique_ptr<MatrixHolder>"]
    A --> C["m_Vectors\nunique_ptr<vector<unique_ptr<vnl_vector>>>"]
    A --> D["m_Solutions\nunique_ptr<vector<unique_ptr<vnl_vector>>>"]
    B --> B1["MatrixHolder\nvector<unique_ptr<vnl_matrix<Float>>>"]
    B1 --> B2["unique_ptr<MatrixRepresentation> [0..N]"]
    C --> C1["unique_ptr<vnl_vector<Float>> [0..N]"]
    D --> D1["unique_ptr<vnl_vector<Float>> [0..N]"]
    style A fill:#4a90d9,color:#fff
    style B fill:#7ec8a0,color:#000
    style C fill:#7ec8a0,color:#000
    style D fill:#7ec8a0,color:#000

Loading

_{Reviews (1): Last reviewed commit: "COMP: Use std::unique_ptr in FEMLinearSy..." | Re-trigger Greptile}

[N-Dekker]

Thanks Hans! Almost perfect. 😇 Just one change request: Please move VectorHolder to the private section of the class.

[N-Dekker]

This PR is clearly an improvement. However, I wonder if the extra pointer indirection is really necessary! Why not simply using std::vector<vnl_matrix<Float>> and std::vector<vnl_vector<Float>> ?

[hjmjohnson]

This PR is clearly an improvement. However, I wonder if the extra pointer indirection is really necessary! Why not simply using std::vector<vnl_matrix<Float>> and std::vector<vnl_vector<Float>> ?

That change should be investigated and performed as a separate PR that builds upon this PR. It is a change outside the scope of what was being worked on here.

[hjmjohnson]

Addressed @N-Dekker's review: moved VectorHolder to the private section. Also moved MatrixRepresentation and MatrixHolder to private in a follow-on commit, since they are equally internal-only and not part of the public API.

[hjmjohnson]

@N-Dekker I had a change of heart. In the pursuit to make this perfect, I want to keep the entire conversation together. When moving the VectorHolder (new alias) to private, it raised the question about MatrixRepresentation and MatrixHolder (ITKv5 public API aliases). I had a migration guide entry generated and moved those two items to private as well. I would guess that those were not intended to be part of the public interface.

[N-Dekker]

Thanks Hans, I agree with this change: making MatrixRepresentation and MatrixHolder private. Note that the "official way" would be to deprecate and future-legacy-remove them. But these two typedefs were clearly never meant to be part of the API.

My change request to make VectorHolder private is addressed

[N-Dekker]

Perfect 💯