Skip to content

COMP: Replace raw new[] with std::vector in CellInterfaceTest#5996

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:cpp-cg-cellinterface-vector
Apr 1, 2026
Merged

COMP: Replace raw new[] with std::vector in CellInterfaceTest#5996
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:cpp-cg-cellinterface-vector

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Summary

Replace raw new[]/delete[] array allocation with std::vector in itkCellInterfaceTest.cxx, following C++ Core Guidelines R.11 (avoid calling new/delete) and ES.27 (use std::array or stack_array for arrays on the stack).

  • Replace new PointIdentifier[N] with std::vector<PointIdentifier>(N)
  • Use std::iota to initialize the sequence, replacing the manual loop
  • Use .data() and .data() + offset for the pointer-based SetPointIds calls
  • Remove delete[] pointIds — memory managed automatically by std::vector
  • Add <numeric> and <vector> includes

Test plan

  • CI builds pass on all platforms
  • itkCellInterfaceTest passes (ctest -R itkCellInterface)

Part of a series splitting out #5993

🤖 Generated with Claude Code

@github-actions github-actions bot added type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Numerics Issues affecting the Numerics module labels Mar 31, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review April 1, 2026 00:38
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 1, 2026

Greptile Summary

This PR replaces raw new[]/delete[] array management with RAII alternatives (std::vector, std::unique_ptr) across itkCellInterfaceTest.cxx and itkFEMLinearSystemWrapperDenseVNL, following the C++ Core Guidelines. The changes are mechanically correct and represent a meaningful quality improvement.

Key highlights:

  • itkCellInterfaceTest.cxx: new PointIdentifier[N] replaced with std::vector<PointIdentifier>(N) initialised via std::iota; <numeric> and <vector> are properly added.
  • itkFEMLinearSystemWrapperDenseVNL.h: MatrixHolder, m_Vectors, and m_Solutions members upgraded from raw-pointer-to-container to std::unique_ptr<container-of-unique_ptr>; destructor simplified to = default. <memory> is not explicitly included despite std::unique_ptr now being used throughout the header (see inline comment).
  • itkFEMLinearSystemWrapperDenseVNL.cxx: The manual destructor (which called virtual DestroyMatrix/DestroyVector/DestroySolution — an anti-pattern) is removed entirely; std::swap is used for pointer-swap operations, which is both correct and more efficient than the previous content-copy approach for vectors.

Confidence Score: 5/5

  • Safe to merge — all changes are semantics-preserving modernisation with no logic regressions.
  • The only finding is a P2 style issue (missing #include <memory> in the header). All raw-pointer-to-unique_ptr conversions are correct, the destructor removal is sound because the unique_ptr chain handles cleanup automatically, and the std::swap replacements preserve observable behaviour. No P0 or P1 issues were found.
  • Modules/Numerics/FEM/include/itkFEMLinearSystemWrapperDenseVNL.h should gain an explicit #include <memory>.

Important Files Changed

Filename Overview
Modules/Core/Mesh/test/itkCellInterfaceTest.cxx Clean replacement of raw new[]/delete[] with std::vector + std::iota; adds required <numeric> and <vector> includes. Logic and pointer usage are correct.
Modules/Numerics/FEM/include/itkFEMLinearSystemWrapperDenseVNL.h Modernises raw-pointer members to std::unique_ptr; destructor simplified to = default. Missing #include <memory> for the newly introduced std::unique_ptr usage.
Modules/Numerics/FEM/src/itkFEMLinearSystemWrapperDenseVNL.cxx All new/delete replaced with make_unique/reset; destructor body removed; std::swap used for pointer-level swaps. Semantics are preserved and the virtual-call-in-destructor anti-pattern is eliminated.

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class LinearSystemWrapperDenseVNL {
        -unique_ptr~MatrixHolder~ m_Matrices
        -unique_ptr~vector~unique_ptr~vnl_vector~Float~~~~ m_Vectors
        -unique_ptr~vector~unique_ptr~vnl_vector~Float~~~~ m_Solutions
        +InitializeMatrix(idx)
        +DestroyMatrix(idx)
        +InitializeVector(idx)
        +DestroyVector(idx)
        +InitializeSolution(idx)
        +DestroySolution(idx)
        +SwapMatrices(i,j)
        +SwapVectors(i,j)
        +SwapSolutions(i,j)
        +CopySolution2Vector(si,vi)
        +CopyVector2Solution(vi,si)
        +Solve()
        +~LinearSystemWrapperDenseVNL() = default
    }

    class MatrixHolder {
        <<type alias>>
        vector~unique_ptr~vnl_matrix~Float~~~
    }

    LinearSystemWrapperDenseVNL --> MatrixHolder : owns via unique_ptr
    LinearSystemWrapperDenseVNL --|> LinearSystemWrapper
Loading

Reviews (1): Last reviewed commit: "COMP: Replace raw new[] with std::vector..." | Re-trigger Greptile

Comment thread Modules/Numerics/FEM/include/itkFEMLinearSystemWrapperDenseVNL.h Outdated
- Replace PointIdentifier array allocation with std::vector
- Use .data() method for passing to SetPointIds

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hjmjohnson hjmjohnson force-pushed the cpp-cg-cellinterface-vector branch from 780311e to 6ff3e29 Compare April 1, 2026 01:19
@github-actions github-actions bot removed the area:Numerics Issues affecting the Numerics module label Apr 1, 2026
@hjmjohnson hjmjohnson enabled auto-merge April 1, 2026 12:30
@hjmjohnson hjmjohnson merged commit d608a14 into InsightSoftwareConsortium:main Apr 1, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module type:Compiler Compiler support or related warnings type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants