Skip to content

STYLE: Remove unused m_UserSpecifiedImageIO from writers#6648

Open
blowekamp wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
blowekamp:removeUnusedUserSpecifiedImageIO-clean
Open

STYLE: Remove unused m_UserSpecifiedImageIO from writers#6648
blowekamp wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
blowekamp:removeUnusedUserSpecifiedImageIO-clean

Conversation

@blowekamp

Copy link
Copy Markdown
Member

Remove the m_UserSpecifiedImageIO member from ImageFileWriter and ImageSeriesWriter. The member was declared in both classes but never read or written in their implementations (itkImageFileWriter.hxx / itkImageSeriesWriter.hxx).

AI assistance

GitHub Copilot (Claude Sonnet 4.6) identified the unused members via workspace-wide grep, verified no usages existed in the implementation files, removed the declarations, and committed the change.

@github-actions github-actions Bot added area:IO Issues affecting the IO module type:Style Style changes: no logic impact (indentation, comments, naming) labels Jul 15, 2026
@hjmjohnson

hjmjohnson commented Jul 15, 2026

Copy link
Copy Markdown
Member

I investigated the failing job and the first fatal error is a compile break in ImageSeriesWriter::PrintSelf.

Error points to Modules/IO/ImageBase/include/itkImageSeriesWriter.hxx (around line ~348), where a PrintSelf macro expands to m_UserSpecifiedImageIO, but that member no longer exists:
error: ... has no member named 'm_UserSpecifiedImageIO' (from itkMacro.h print macro expansion).

Recommended fix: update PrintSelf to stop referencing the removed/renamed ivar.

Remove itkPrintSelfBooleanMacro(UserSpecifiedImageIO);

This should unblock the failing compile target:
Modules/IO/GDCM/test/CMakeFiles/ITKIOGDCMTestDriver.dir/itkGDCMImageReadSeriesWriteTest.cxx.o

This is only the first failure, possible others.

The member was declared in ImageFileWriter and ImageSeriesWriter but
never read or written in their implementations.
@blowekamp
blowekamp force-pushed the removeUnusedUserSpecifiedImageIO-clean branch from a8f1a11 to 1f7421a Compare July 16, 2026 13:53
@blowekamp

Copy link
Copy Markdown
Member Author

Fixed the compilation error by removing itkPrintSelfBooleanMacro(UserSpecifiedImageIO); from itkImageSeriesWriter.hxx, per the review comment. Full build completed successfully with 34/34 related tests passing.

@blowekamp
blowekamp marked this pull request as ready for review July 16, 2026 18:07
@N-Dekker

Copy link
Copy Markdown
Contributor

Just wondering, why was m_UserSpecifiedImageIO added, originally? Looks like it was still used in the past.

@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes unused image-I/O tracking state from two writer classes. The main changes are:

  • Removes the unused private member from ImageFileWriter.
  • Removes the unused protected member from ImageSeriesWriter.
  • Removes the corresponding ImageSeriesWriter::PrintSelf field.

Confidence Score: 5/5

This looks safe to merge after the existing review feedback is resolved.

No additional blocking issues were found in the changed code.

Existing review feedback.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex ran two writer toolchain attempts (writer-toolchain-01-before.log and writer-toolchain-02-after.log) to exercise the build steps, and both exited with 127 and the message cmake: not found.
  • T-Rex inspected writer-toolchain-bootstrap.log to capture the concise bootstrap/tool lookup behavior.
  • T-Rex concluded that no compilation or writer actions could be executed because cmake was missing, so there is no runtime path to evaluate.
  • T-Rex prepared artifacts and summaries to help verify the blocking condition across the toolchain logs.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/IO/ImageBase/include/itkImageFileWriter.h Removes an unused private Boolean member without changing writer behavior.
Modules/IO/ImageBase/include/itkImageSeriesWriter.h Removes the unused protected image-I/O tracking member.
Modules/IO/ImageBase/include/itkImageSeriesWriter.hxx Removes the matching Boolean field from PrintSelf output.

Reviews (2): Last reviewed commit: "STYLE: Remove unused m_UserSpecifiedImag..." | Re-trigger Greptile

Comment thread Modules/IO/ImageBase/include/itkImageSeriesWriter.h
Comment thread Modules/IO/ImageBase/include/itkImageSeriesWriter.hxx
@hjmjohnson

hjmjohnson commented Jul 17, 2026

Copy link
Copy Markdown
Member

@N-Dekker It was used in ImageFileReader, and still is —ImageFileReader is a different class with its own member of the same name, untouched by this PR. In the writers it wasn't merely unused: it was superseded by m_FactorySpecifiedImageIO, which tracks the same fact with the opposite polarity (user == !factory). The inverted name is probably why the orphan survived — grepping for one never finds the other.

On @greptile-apps' open issue (protected-member removal breaking external subclasses): I searched a downstream testbed of 31 ITK-consumer source trees built against ITK main — no consumers of the removed member, and no subclasses of ImageSeriesWriter at all. Greptile's concern is sound in principle, but T-Rex's reproduction uses a synthetic subclass; I could not find a real one in that corpus, in or out of tree.

Where the logic actually lives (answers "why was it added")

ImageFileReader uses its copy to decide whether it may replace the IO via the factory:

// itkImageFileReader.hxx:67   (SetImageIO)
m_UserSpecifiedImageIO = true;

// itkImageFileReader.hxx:100  (GenerateOutputInformation)
if (m_UserSpecifiedImageIO == false) // try creating via factory
{
  m_ImageIO = ImageIOFactory::CreateImageIO(this->GetFileName().c_str(), ReadMode);
}

m_ImageIO alone cannot answer that — a user-set IO and a factory-made one are both non-null — so the flag is load-bearing there.

ImageFileWriter answers the same question from the factory side, as Greptile also noted:

// itkImageFileWriter.h:234
bool m_FactorySpecifiedImageIO{ false }; // did factory mechanism set the ImageIO?
// itkImageFileWriter.h:139    (SetImageIO)
m_FactorySpecifiedImageIO = false;
// itkImageFileWriter.hxx:99
if (m_ImageIO.IsNull() || (m_FactorySpecifiedImageIO && !m_ImageIO->CanWriteFile(m_FileName.c_str())))
{
  m_ImageIO = ImageIOFactory::CreateImageIO(m_FileName.c_str(), WriteMode);
  m_FactorySpecifiedImageIO = true;   // hxx:112
}

The writer's form is the stronger of the two: it re-runs the factory when a factory-made IO !CanWriteFile() the current filename, so it also handles a format change on a new filename.

(Line numbers from ITK main @ 0acef9a.)

Downstream search: method and result

Corpus: 31 ITK-consumer source trees checked out and built against ITK main in a downstream build testbed — ANTs, elastix, Slicer, BRAINSTools, SimpleITK, RTK, MITK, and the ITK remote modules.

  • ImageFileWriter::m_UserSpecifiedImageIO is private — unreachable downstream regardless.
  • ImageSeriesWriter::m_UserSpecifiedImageIO is protected, so it is genuinely API surface — but there are no subclasses of ImageSeriesWriter in the corpus, nor in ITK itself.
  • The only external subclass of either writer is elastix's ImageFileCastWriter : public ImageFileWriter, which never references the member (and could not — private).
  • ANTs matches are false positives: its VectorImageFileReader/VectorImageFileWriter derive from ImageSource/ProcessObject, not from these classes, and declare their own m_UserSpecifiedImageIO — carrying the reader's original comment verbatim, which suggests where the writers' copies came from too.

Caveat: 31 consumers is not the whole ecosystem; a private or unlisted downstream subclass of ImageSeriesWriter would not appear here.

Possibly out of scope: ImageSeriesWriter has no provenance flag at all

ImageSeriesWriter has neither m_UserSpecifiedImageIO logic nor m_FactorySpecifiedImageIO (0 references in its .h/.hxx), so unlike ImageFileWriter it appears to have no user-vs-factory tracking whatsoever. That looks orthogonal to this cleanup, but may be worth its own look.

@hjmjohnson

Copy link
Copy Markdown
Member

@blowekamp In its current state, this is dead code (or at least not fully gestated code). I think removing it is the correct choice. I did wonder if making it symmetric with the itkImageFileReader made sense. Still, given the 20-year history of being a copy-and-paste addition, I don't think there is a compelling need to add the requisite logic to keep this alive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:IO Issues affecting the IO module type:Style Style changes: no logic impact (indentation, comments, naming)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants