Skip to content

COMP: Replace raw pointers with std::unique_ptr in VoxBoCUBImageIO#5997

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:cpp-cg-voxbocub-unique-ptr
Apr 1, 2026
Merged

COMP: Replace raw pointers with std::unique_ptr in VoxBoCUBImageIO#5997
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
hjmjohnson:cpp-cg-voxbocub-unique-ptr

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Summary

Replace raw pointer ownership in VoxBoCUBImageIO with std::unique_ptr, following C++ Core Guidelines R.11 and R.20.

  • Change m_Reader and m_Writer members from GenericCUBFileAdaptor* to std::unique_ptr<GenericCUBFileAdaptor>
  • Change CreateReader/CreateWriter return types to std::unique_ptr<GenericCUBFileAdaptor>; use std::make_unique for polymorphic factory creation
  • Default the destructor — no manual delete needed
  • Call m_Writer.reset() at end of Write() to preserve the original close-on-write behaviour (flush/close the file immediately after writing, matching the original delete m_Writer call)
  • Remove {} brace-initializers from the unique_ptr members to avoid a GCC 7 bug with brace-initialization of smart pointers to forward-declared classes (reported in COMP: Remove in-class {} member initializers of unique_ptr #3877)
  • Add <memory> include

Test plan

  • CI builds pass on all platforms (including GCC 7)
  • VoxBoCUB IO tests pass (ctest -R VoxBo)

Part of a series splitting out #5993
Addresses review comment from @N-Dekker regarding GCC 7 compatibility of {} on smart pointers to forward-declared types

🤖 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
@N-Dekker
Copy link
Copy Markdown
Contributor

Thanks for splitting #5993, Hans!

Can you please remove the first two commits from this PR?

@N-Dekker
Copy link
Copy Markdown
Contributor

@hjmjohnson Please note that VoxBoCUBImageIO is in the "Nonunit/Review" module, so it may not be tested directly by the CI. Did you test it locally?

@hjmjohnson
Copy link
Copy Markdown
Member Author

@hjmjohnson Please note that VoxBoCUBImageIO is in the "Nonunit/Review" module, so it may not be tested directly by the CI. Did you test it locally?

Yes. I explicitly required a build and test in my local environment.

- Change m_Reader/m_Writer from GenericCUBFileAdaptor* to
  std::unique_ptr<GenericCUBFileAdaptor>
- Change CreateReader/CreateWriter return types to std::unique_ptr;
  use std::make_unique for polymorphic factory creation
- Call m_Writer.reset() at end of Write() to preserve original
  close-on-write behavior (flush/close file immediately)
- Remove {} brace-initializers from unique_ptr members: GCC 7 has a
  known bug (fixed in GCC 9.2) where brace-initializing a smart pointer
  to a forward-declared class instantiates ~unique_ptr<T> with an
  incomplete type. See ITK#3877.
- Declare ~VoxBoCUBImageIO() in the header and define it as = default
  in the .cxx file where GenericCUBFileAdaptor is a complete type.
  Defining = default in the header would instantiate
  ~unique_ptr<GenericCUBFileAdaptor> with an incomplete type, causing
  undefined behavior. See https://andreasfertig.com/blog/2023/12/when-an-empty-destructor-is-required/
- Add <memory> include
@hjmjohnson hjmjohnson force-pushed the cpp-cg-voxbocub-unique-ptr branch from eb58cc8 to 6c699a6 Compare April 1, 2026 02:57
@github-actions github-actions bot removed 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 Apr 1, 2026
@hjmjohnson hjmjohnson marked this pull request as ready for review April 1, 2026 12:08
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 1, 2026

Greptile Summary

This PR modernises VoxBoCUBImageIO by replacing two raw-pointer owning members (m_Reader, m_Writer) with std::unique_ptr<GenericCUBFileAdaptor>, updating the factory methods CreateReader/CreateWriter to return std::unique_ptr, and deleting all manual new/delete calls. The refactoring follows C++ Core Guidelines R.11/R.20 and is otherwise a pure mechanical transformation.

  • <memory> correctly added to the header.
  • Member declarations drop {} brace-initializers intentionally to avoid a GCC 7 bug (described in COMP: Remove in-class {} member initializers of unique_ptr #3877); std::unique_ptr is still value-initialised to nullptr by default — semantically equivalent.
  • The destructor is declared override in the header but defined = default in the .cxx, where GenericCUBFileAdaptor is a complete type. The explanatory comment is accurate and important: placing = default in the header would instantiate ~unique_ptr<GenericCUBFileAdaptor> with an incomplete type.
  • m_Writer.reset() at the end of Write() exactly preserves the original close-on-write behaviour of delete m_Writer.
  • Exception-safety is improved over the original: if WriteImageInformation() or WriteData() throw, the file handle is now properly cleaned up when the VoxBoCUBImageIO object is eventually destroyed, rather than leaking as it did before.

Confidence Score: 5/5

Safe to merge — the refactoring is mechanically correct, semantically equivalent, and improves exception safety.

All changes are a well-understood raw-pointer → unique_ptr migration. Destructor placement, factory return types, member initialisation, and reset semantics are all handled correctly. No logic is altered; exception safety is strictly improved. No P0/P1 findings.

No files require special attention.

Important Files Changed

Filename Overview
Modules/Nonunit/Review/include/itkVoxBoCUBImageIO.h Adds <memory>, changes m_Reader/m_Writer to std::unique_ptr, updates CreateReader/CreateWriter signatures; destructor kept declared but deferred to .cxx — all correct.
Modules/Nonunit/Review/src/itkVoxBoCUBImageIO.cxx Destructor correctly defined as = default in .cxx (complete-type context), std::make_unique replaces new, all manual delete calls removed, m_Writer.reset() preserves close-on-write semantics — clean and correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[VoxBoCUBImageIO created] --> B[m_Reader = nullptr
m_Writer = nullptr
via unique_ptr default init]

    B --> C{CanReadFile called}
    C --> D[local reader = CreateReader
scoped unique_ptr]
    D --> E{reader == nullptr?}
    E -- yes --> F[return false]
    E -- no --> G[check header content]
    G --> H[reader auto-destroyed
on scope exit]
    H --> I[return iscub]

    B --> J{ReadImageInformation called}
    J --> K[m_Reader = CreateReader
releases old reader]
    K --> L{m_Reader == nullptr?}
    L -- yes --> M[throw exception]
    L -- no --> N[read header & parse metadata
m_Reader kept alive]

    B --> O{Write called}
    O --> P[m_Writer = CreateWriter
releases any old writer]
    P --> Q[WriteImageInformation
uses m_Writer]
    Q --> R[m_Writer->WriteData]
    R --> S[m_Writer.reset
flushes & closes file]

    B --> T[VoxBoCUBImageIO destroyed]
    T --> U[~unique_ptr destroys
m_Reader and m_Writer
if still set]
Loading

Reviews (1): Last reviewed commit: "COMP: Replace raw pointers with std::uni..." | Re-trigger Greptile

@hjmjohnson hjmjohnson requested a review from N-Dekker April 1, 2026 12:29
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker left a comment

Choose a reason for hiding this comment

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

Thanks Hans! Great!

@hjmjohnson hjmjohnson merged commit 9fb0783 into InsightSoftwareConsortium:main Apr 1, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:Compiler Compiler support or related warnings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants