Skip to content

COMP: Update DCMTK to add printf format attribute in offile.h - #6587

Closed
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:comp-dcmtk-format-nonliteral
Closed

COMP: Update DCMTK to add printf format attribute in offile.h#6587
hjmjohnson wants to merge 1 commit into
InsightSoftwareConsortium:mainfrom
hjmjohnson:comp-dcmtk-format-nonliteral

Conversation

@hjmjohnson

Copy link
Copy Markdown
Member

Updates the vendored DCMTK to add __attribute__((format(printf, ...))) to OFFile::fprintf/OFFile::vfprintf, fixing two clang -Wformat-nonliteral warnings emitted in every ITK translation unit that includes offile.h (e.g. via itkDCMTKFileReader.h).

Details
  • Fork commit: InsightSoftwareConsortium/DCMTK@84c8047, fast-forwarded onto for/itk-dcmtk-3.7.0-ccfd10b.
  • The attribute marks the format parameter as a valid forwarding target (silencing the wrapper-internal false positive) and enables format/argument checking at all call sites.
  • Chosen over pragma suppression so the fix is acceptable to upstream DCMTK, whose master has the same code; the same patch is being submitted upstream.
  • Verified locally: full ITKIODCMTKTestDriver rebuild (all of DCMTK + ITK IODCMTK tests) against the new pin with zero warnings/errors.

@github-actions github-actions Bot added type:Compiler Compiler support or related warnings area:ThirdParty Issues affecting the ThirdParty module labels Jul 10, 2026
OFFile::fprintf and OFFile::vfprintf forward a non-literal format
string to ::vfprintf, warning in every ITK translation unit that
includes offile.h. The format attribute silences the false positive
and adds format/argument checking at call sites.
@hjmjohnson
hjmjohnson force-pushed the comp-dcmtk-format-nonliteral branch from 8582e46 to 2300b0b Compare July 10, 2026 22:29
@hjmjohnson
hjmjohnson marked this pull request as ready for review July 11, 2026 18:28
@greptile-apps

greptile-apps Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR updates the vendored DCMTK pin for a compile-warning fix. The main changes are:

  • Advances the DCMTK fork commit hash on the same ITK DCMTK 3.7.0 branch.
  • Documents the added OFFile printf-format attribute patch in the local patch list.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the changed code.

No files need attention.

T-Rex T-Rex Logs

What T-Rex did

  • A post-change DCMTK pin validation run was executed, and the after-log captures missing build tools, the parsed URL and tag, a successful curl fetch, and header snippets showing the format(printf, ...) macro and annotations.
  • A validation script named dcmtk-pin-validation-script.sh was generated and used to reproduce the validation run that produced the after-log.
  • There was no pre-change baseline checkout available in this sandbox, so no before-run comparison could be performed.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
Modules/ThirdParty/DCMTK/DCMTKGitTag.cmake Updates the DCMTK FetchContent commit pin and patch-list comment while keeping the same release-anchored fork branch.

Reviews (1): Last reviewed commit: "COMP: Update DCMTK to add printf format ..." | Re-trigger Greptile

@hjmjohnson
hjmjohnson requested a review from blowekamp July 12, 2026 21:56
@hjmjohnson

Copy link
Copy Markdown
Member Author

Closing as obsolete — this patch already landed on main via #6548, which advanced the DCMTK pin past the commit this PR pins.

Why this is superseded (and why rebasing it would regress main)

This PR pins DCMTK at 84c8047d ("COMP: Add printf format attribute to OFFile printf wrappers"). #6548 moved main to 554b7447, one commit further along the same for/itk-dcmtk-3.7.0-ccfd10b branch:

554b7447  COMP: Use single export() call to fix namespace corruption   <- main's pin
84c8047d  COMP: Add printf format attribute to OFFile printf wrappers  <- this PR's pin
4ac9b448  Merge pull request #2 from blowekamp/use_dcmtk_override

84c8047d is an ancestor of 554b7447, and ofstd/include/dcmtk/ofstd/offile.h is byte-identical between the two pins — OFFILE_FORMAT_PRINTF is present in main's pinned tree (offile.h:59, applied at :901 and :917).

Because DCMTK_GIT_TAG is a single value, the merge conflict here is not reconcilable by keeping both sides: resolving in favor of this PR would move the pin backwards to 84c8047d and silently revert the namespace-corruption fix. Hence closing rather than rebasing.

The green CI on this PR predates #6548 and reflects the older base.

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

Labels

area:ThirdParty Issues affecting the ThirdParty module type:Compiler Compiler support or related warnings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant