Skip to content

COMP: Apply C++ Core Guidelines smart pointer fixes#5993

Closed
hjmjohnson wants to merge 3 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:cpp-core-guidelines-smart-pointers
Closed

COMP: Apply C++ Core Guidelines smart pointer fixes#5993
hjmjohnson wants to merge 3 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:cpp-core-guidelines-smart-pointers

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson commented Mar 31, 2026

Summary

Replace raw pointer ownership patterns with std::unique_ptr across three modules, following C++ Core Guidelines R.11 (avoid calling new and delete explicitly) and R.20 (use unique_ptr to represent ownership).

  • FEMLinearSystemWrapperDenseVNL: Replace raw new/delete of matrix and vector holders with std::unique_ptr and std::make_unique. Use std::swap for all swap methods. Default the destructor.
  • CellInterfaceTest: Replace raw new[]/delete[] array allocation with std::vector, eliminating manual memory management in test code.
  • VoxBoCUBImageIO: Replace raw pointer return types and members with std::unique_ptr<GenericCUBFileAdaptor>, using std::make_unique for polymorphic factory creation. Default the destructor.

OpenCVVideoIO changes split to separate PR: #5994

Test plan

  • Verify CI builds pass on all platforms (Linux, macOS, Windows)
  • Verify existing CTest tests pass (no behavioral changes, only ownership semantics)
  • Verify FEM module tests pass
  • Verify Mesh/CellInterface test passes
  • Verify VoxBoCUB IO tests pass

🤖 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:Video Issues affecting the Video module area:Numerics Issues affecting the Numerics module labels Mar 31, 2026
@hjmjohnson hjmjohnson marked this pull request as draft March 31, 2026 14:39
@hjmjohnson hjmjohnson force-pushed the cpp-core-guidelines-smart-pointers branch from c0430d0 to 8779182 Compare March 31, 2026 14:51
Replace raw pointer members and new/delete with std::unique_ptr
and std::make_unique. Use std::swap for all Swap* methods.
Destructor is defaulted since unique_ptr handles cleanup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@hjmjohnson hjmjohnson force-pushed the cpp-core-guidelines-smart-pointers branch from 8779182 to 4863383 Compare March 31, 2026 14:56
@github-actions github-actions bot removed the area:Video Issues affecting the Video module label Mar 31, 2026
@hjmjohnson
Copy link
Copy Markdown
Member Author

All issues from the greptile review have been addressed in the force-pushed v3:

  • P0 – Orphaned code at global scope: OpenCVVideoIO changes split out to PR COMP: Migrate OpenCV Video Bridge from legacy C API to C++ API #5994, which was written clean without this bug.
  • P0 – catch (..) typo: Fixed to catch (...) with three dots.
  • P1 – Writer not closed after Write(): m_Writer.reset() now called at end of Write(), preserving original flush/close behavior.
  • P2 – SwapVectors inconsistency: Now uses std::swap on the unique_ptrs, consistent with SwapMatrices and SwapSolutions.

@hjmjohnson hjmjohnson force-pushed the cpp-core-guidelines-smart-pointers branch from ce7029e to 4863383 Compare March 31, 2026 15:30
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Looks good on a glance. I would prefer is someone else reviewed this too.

hjmjohnson and others added 2 commits March 31, 2026 12:19
- Replace PointIdentifier array allocation with std::vector
- Use .data() method for passing to SetPointIds

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace raw pointer return types and members with std::unique_ptr.
Use std::make_unique for polymorphic factory creation. Destructor
is defaulted since unique_ptr handles cleanup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@hjmjohnson hjmjohnson force-pushed the cpp-core-guidelines-smart-pointers branch from 4863383 to 6862133 Compare March 31, 2026 17:19
@hjmjohnson hjmjohnson requested a review from N-Dekker March 31, 2026 17:21
Comment on lines +113 to +114
std::unique_ptr<GenericCUBFileAdaptor> m_Reader{};
std::unique_ptr<GenericCUBFileAdaptor> m_Writer{};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the past, the use of {} appeared problematic for smart pointers to a forward-declared class:

I don't know if that is still relevant nowadays. 🤷 For GCC std::unique_ptr, it was fixed with GCC 9.2. Do we require GCC >= 9.2?

If the {} do still cause a problem, simply remove them. Default-initialization of std::unique_ptr will work fine anyway!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If memory serves me well, we support a rather old GCC version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GCC 7, according to:

* GCC 7 and later [should be supported](https://www.gnu.org/software/gcc/projects/cxx-status.html).

So unfortunately then, these particular {} should be removed!

(The {} problem is specific for smart pointers to a forward-declared class. In other cases, {} usually just works fine.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Working on it now.

@N-Dekker
Copy link
Copy Markdown
Contributor

Sorry, but again, I would prefer these commits to be in separate PRs.

@hjmjohnson
Copy link
Copy Markdown
Member Author

Closing in favor of three focused single-commit PRs:

Each PR contains exactly one commit and can be reviewed and merged independently.

@hjmjohnson hjmjohnson closed this Mar 31, 2026
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 area:Numerics Issues affecting the Numerics 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.

3 participants