BUG: Wrap std::sto* exceptions in IO with itk::ExceptionObject (#3213)

[hjmjohnson]

Fixes #3213 by introducing context-aware itk::StringTo* helpers and migrating non-test std::sto* call sites in the IO modules to use them. Corrupt input now raises an itk::ExceptionObject (with the offending field name and value) rather than a bare std::invalid_argument / std::out_of_range that crashes downstream applications such as Slicer.

Background and rationale

ITK switched from atoi/atof to std::stoi / std::stod in commit b9a384a (2018) for better error checking. As discussed in Slicer/Slicer#6200, that change introduced a behavior regression for callers that catch only itk::ExceptionObject: a corrupt header that previously parsed silently as zero now throws an exception unrelated to ITK's exception hierarchy and crashes the application. The Slicer reviewers asked that ITK catch the low-level conversion exceptions internally and rethrow them as itk::ExceptionObject with a meaningful message — which is what this PR does.

Behavior tiers as framed in that thread:

• WORST: silently produce wrong results
• REALLY BAD: crash with an unhandled std exception
• BEST: throw a typed contextual exception that downstream catchers already handle

Wrapping moves the IO read paths into the BEST tier without changing success-path behavior.

What changed

Commit 1 — ENH: Add itk::StringTo{Int,Long,ULong,Double,Float} helpers

• New header Modules/Core/Common/include/itkStringTools.h and implementation in src/itkStringTools.cxx.
• Each helper takes the input string and a const char * context. On failure it throws itk::ExceptionObject whose message includes the target type, the failure kind (invalid_argument or out_of_range), the context, the offending input (truncated past 64 chars), and the underlying what().
• StringToULong additionally rejects an explicit leading minus sign; std::stoul silently wraps it.
• New GoogleTest itkStringToolsGTest.cxx covers valid round-trip, empty input, non-numeric input, out-of-range overflow, message content, the long-input truncation path, the null-context path, and the explicit-minus rejection.

Commit 2 — BUG: Wrap std::sto call sites in IO with itk::ExceptionObject*

Per-module migration:

• Core/Common — itkSmapsFileParser bracketed size
• IO/Bruker — VisuFGOrderDesc size entries
• IO/GDCM — DICOM (0028,0100/0101/0102/0103)
• IO/GE — SIGNA series and image numbers, images-per-slice
• IO/ImageBase — RegularExpressionSeriesFileNames numeric sort key
• IO/MeshVTK — VTK PolyData header version major
• IO/NIFTI — metadata qform_code and sform_code
• IO/Siemens — Vision header TEXT_* numeric fields
• IO/SpatialObjects — PolygonGroup XML reader
• Video/IO — VideoIOFactory camera index

Plus a new corruption test itkPolygonGroupXMLMalformedTest that feeds malformed XML and asserts the failure surfaces as itk::ExceptionObject (not std::exception), locking in the wrapping contract end-to-end for at least one IO module.

Scope notes (what is intentionally NOT touched)

• Test-side std::sto* calls (parsing argv in CTest drivers; roughly 60 sites) are intentionally left as-is. A stack trace from a developer-supplied bad command-line argument is more diagnostic than a wrapped exception; the Slicer reviewers explicitly noted that test-side crashes are desirable.
• No silent-recovery substitution. Where a caller legitimately wants empty-string-means-zero behavior (the old atoi semantics) that is the caller's policy and should be expressed as an explicit empty-string check before calling the helper. The Siemens reader already does this for text_angle_len; this PR preserves that pattern.
• ThirdParty trees and Examples are untouched.

Test plan

• CI green across configured platforms
• ITKCommonGTestDriver --gtest_filter=StringTools.* passes
• itkPolygonGroupXMLMalformedTest passes (asserts wrapped exception)
• Existing PolygonGroup / NIfTI / GDCM read tests still pass on valid input

[greptile-apps]

Greptile Summary

This PR introduces five itk::StringTo* helpers that wrap std::sto* conversion failures as itk::ExceptionObject, then migrates all non-test IO call sites to use them. The core implementation is clean and well-tested; all remaining findings are P2 or lower and do not block merge.

Confidence Score: 5/5

Safe to merge; all findings are P2 suggestions or pre-existing issues not worsened by this PR.

The new helpers are correctly implemented with [[noreturn]] semantics, input truncation, and a targeted ULong leading-minus guard. The IO migration is systematic. The only noteworthy points are: a pre-existing throwing-comparator UB in RegularExpressionSeriesFileNames (not introduced here), a subtle stod→stof behavior change for out-of-float-range doubles (strictly more correct), and a missing ULong overflow unit test. None of these block merge.

Modules/IO/ImageBase/src/itkRegularExpressionSeriesFileNames.cxx (pre-existing throwing comparator UB worth a follow-up); Modules/IO/SpatialObjects/src/itkPolygonGroupSpatialObjectXMLFile.cxx (stod→stof behavior note).

Important Files Changed

Filename	Overview
Modules/Core/Common/include/itkStringTools.h	New header declaring five ITKCommon-exported string-to-numeric helpers with well-documented context-aware exception wrapping.
Modules/Core/Common/src/itkStringTools.cxx	Solid implementation; [[noreturn]] throwParseFailure correctly prevents "falls off end" UB; StringToULong leading-minus loop is correct; input truncation at 64 chars prevents large-message attacks.
Modules/Core/Common/test/itkStringToolsGTest.cxx	Good coverage of valid, invalid, overflow, truncation, null-context, and minus-rejection cases; missing an explicit StringToULong overflow test to match StringToInt parity.
Modules/IO/ImageBase/src/itkRegularExpressionSeriesFileNames.cxx	StringToDouble migrated correctly; however, the comparator throws inside std::sort (pre-existing UB, not worsened by this PR).
Modules/IO/SpatialObjects/src/itkPolygonGroupSpatialObjectXMLFile.cxx	Migration from std::stoi/stod to StringToInt/Float is correct; subtle behavior change: stod→stof for RESOLUTION fields now throws instead of silently producing infinity for out-of-float-range doubles.
Modules/IO/SpatialObjects/test/itkPolygonGroupXMLMalformedTest.cxx	New end-to-end regression test correctly verifies that malformed numeric XML surfaces as itk::ExceptionObject, not std::exception.
Modules/Video/IO/src/itkVideoIOFactory.cxx	StringToInt replaces std::stoi for camera index; minor: conversion is redundantly called per-loop-iteration (pre-existing pattern).
Modules/IO/GDCM/src/itkGDCMImageIO.cxx	DICOM pixel-depth fields migrated from std::stoi to StringToInt with informative contexts; straightforward mechanical migration.
Modules/IO/Siemens/src/itkSiemensVisionImageIO.cxx	TEXT_ANGLE empty-string guard preserved; TEXT_SLICE_POS and TEXT_ECHO_NUM now throw itk::ExceptionObject on malformed data instead of std::invalid_argument.
Modules/IO/NIFTI/src/itkNiftiImageIO.cxx	qform_code and sform_code metadata parsing migrated to StringToInt; clean migration with meaningful context strings.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["IO Reader calls std::sto*"] --> B{Conversion succeeds?}
    B -- Yes --> C[Return parsed value]
    B -- "No (invalid_argument or out_of_range)" --> D["itk::StringTo* helper catches std::exception"]
    D --> E["throwParseFailure() [[noreturn]]"]
    E --> F["itkGenericExceptionMacro throws itk::ExceptionObject with context + offending input"]
    F --> G["Downstream catch(itk::ExceptionObject) in Slicer / application"]
    G --> H[Graceful error handling]

    style A fill:#f9f,stroke:#333
    style F fill:#f96,stroke:#333
    style H fill:#6f6,stroke:#333

Loading

_{Reviews (1): Last reviewed commit: "BUG: Wrap std::sto* call sites in IO wit..." | Re-trigger Greptile}

[dzenanz]

The fix looks good on a glance. I did not take a good look at the tests.

[hjmjohnson]

Renamed itk::StringToULong → itk::StringToUInt64 (returns std::uint64_t, internally std::stoull). Force-pushed as 3ad9e63435.

Why: unsigned long is 32 bits on 64-bit Windows (LLP64) and 64 bits on Linux/macOS (LP64). For an IO helper that may parse header-field sizes (e.g., NIfTI > 4 GB), unsigned long would silently throw out_of_range on Windows for values that fit comfortably in 64 bits everywhere else. std::uint64_t is exactly 64 bits on every platform.

Internally: std::stoull returns unsigned long long (≥ 64 bits per the standard); cast to std::uint64_t for fixed-width semantics.

Tests added: explicit 9999999999 (> 2^32) round-trip case to lock in the cross-platform width contract; 2^64 = 18446744073709551616 overflow case mirroring the signed test.

No callers outside this PR's own GTest, so the rename is zero-blast-radius. Pre-commit passes locally; CI re-running on the new HEAD.

[hjmjohnson]

Refactored to consistent fixed-width naming as suggested. Force-pushed as bc3ee656f9.

API now:

• std::int32_t itk::StringToInt32(...)
• std::int64_t itk::StringToInt64(...)
• std::uint32_t itk::StringToUInt32(...)
• std::uint64_t itk::StringToUInt64(...)
• double itk::StringToDouble(...) (unchanged)
• float itk::StringToFloat(...) (unchanged)

Same width-portability concern as unsigned long applied to long (LP64 vs LLP64 — long is 32-bit on Windows x64, 64-bit on Linux/macOS), so the rename was extended.

Implementation:

• StringToInt32/StringToUInt32 use std::stoll/std::stoull and explicitly range-check against numeric_limits<int32_t>::min/max / numeric_limits<uint32_t>::max before casting. The check fires before the cast, so a value that fits in the underlying long long but not in the named width throws out_of_range with a clear message rather than truncating.
• StringToInt64/StringToUInt64 use std::stoll/std::stoull directly (≥ 64-bit guaranteed by the standard) and cast to fixed-width.
• The leading-minus-sign guard for unsigned helpers is factored into a single rejectLeadingMinus() helper used by both UInt32 and UInt64.

Tests: all six functions get parity coverage — valid round-trip including boundary values (INT32_MAX, INT64_MAX, UINT32_MAX, UINT64_MAX), empty / non-numeric / out-of-range, message content, leading-minus rejection for unsigned, and the 512-char truncation contract.

IO consumers updated: itk::StringToInt( → itk::StringToInt32( in 9 files (Smaps, Bruker, GDCM, GE, ImageBase, MeshVTK, NIFTI, Siemens, SpatialObjects, Video/IO). All call sites previously cast the int result to unsigned short / SizeValueType etc., which is unchanged.

Pre-commit clean locally; CI re-running on the new HEAD.

[hjmjohnson]

@N-Dekker Thank you for catching this. It is an improvement over the historical code!

[hjmjohnson]

dzenanz
Approved these changes