InsightSoftwareConsortium / InsightSoftwareConsortium/ITK
STYLE: Use C++17 structured bindings for map/pair iteration
- Dominant language
- C++
- Stars
- 1.7k
- Forks
- 748
- Avg merge
- 1d 1h
- Merged PRs (30d)
- 64
Description
26 range-for loops in ITK iterate a map/pair container and then immediately read `.first` / `.second` off the loop variable — textbook C++17 structured-binding sites. The win is naming and readability; codegen is identical.
Two of the 26 iterate **by value**, and one of those, `itkLabelOverlapMeasuresImageFilter.hxx:52`, sits in a hot merge path — that one is a genuine (small) performance defect and should land first, independently of the cosmetic sweep.
What the change looks like
```cpp
// before
for (const auto & kv : m_ScalarDictionary)
{
use(kv.first);
use(kv.second);
}
// after
for (const auto & [key, value] : m_ScalarDictionary)
{
use(key);
use(value);
}
```
Several loop variables are literally named `pair`, `kv`, `keyval`, `mapEntry`, `sizePair`, `sizeVectorPair` — placeholder names that exist only because the destructure was unavailable.
Structured bindings themselves have **zero** performance impact: the compiler synthesizes a hidden `__e` initialized from the loop element, and `[a, b]` names alias `get<0>(__e)` / `get<1>(__e)`, which for `std::pair` reduce to `.first` / `.second`. Identical codegen at every optimization level on every mainstream compiler.
Full inventory — 26 sites in 15 files (verified against main 2026-07-29)
| File | Line(s) | Count |
|---|---|---:|
| `Modules/Core/Common/src/itkProcessObject.cxx` | 109, 634, 664, 782, 957, 1201, 1245, 1745, 1763 | 9 |
| `Modules/Core/Common/src/itkAnatomicalOrientation.cxx` | 178 | 1 |
| `Modules/Core/Common/src/itkMetaDataDictionary.cxx` | 47 | 1 |
| `Modules/Core/Common/src/itkObjectFactoryBase.cxx` | 611 | 1 |
| `Modules/Core/Common/test/itkBuildInformationGTest.cxx` | 50 | 1 |
| `Modules/Core/SpatialObjects/src/itkSpatialObjectProperty.cxx` | 196, 202 | 2 |
| `Modules/Core/SpatialObjects/include/itkSpatialObjectPoint.hxx` | 136 | 1 |
| `Modules/Core/Mesh/test/itkMeshTest.cxx` | 205 | 1 |
| `Modules/Filtering/ImageStatistics/include/itkLabelStatisticsImageFilter.hxx` | 57, 467 | 2 |
| `Modules/Filtering/ImageStatistics/include/itkLabelOverlapMeasuresImageFilter.hxx` | 52 | 1 |
| `Modules/Filtering/ImageCompose/test/itkComposeBigVectorImageFilterTest.cxx` | 84 | 1 |
| `Modules/Segmentation/ConnectedComponents/include/itkRelabelComponentImageFilter.hxx` | 115, 178 | 2 |
| `Modules/IO/XML/src/itkDOMNodeXMLWriter.cxx` | 65 | 1 |
| `Modules/Nonunit/Review/src/itkVoxBoCUBImageIO.cxx` | 834 | 1 |
| `Examples/IO/XML/itkParticleSwarmOptimizerDOMWriter.cxx` | 74 | 1 |
**Highest-leverage single file:** `itkProcessObject.cxx` — 9 of the 26. Every loop is over `m_Inputs` or `m_Outputs` (name-string → `DataObject` pointer), so `input.first` is a name and `input.second` a pointer; `for (auto & [name, dataObj] : m_Inputs)` is a strict readability gain.
**The 2 by-value loops** (`for (auto var : container)`, no `&` — copies the pair every iteration):
| File:line | Hot path? | Notes |
|---|---|---|
| `itkLabelOverlapMeasuresImageFilter.hxx:52` (`auto m2_value`) | **yes** — inside `MergeMap`, which runs after every threaded chunk | With a 500-label image, 8 stream divisions and 16 threads this is ~64 000 avoidable pair copies per `Update()`. |
| `itkComposeBigVectorImageFilterTest.cxx:84` | no — test only | Negligible; fold in for consistency. |
Conversion rules and the one non-obvious case
- **Give the bindings meaningful names.** If the original was `kv` or `pair`, use `[label, value]` — never `[kv_first, kv_second]`, which defeats the purpose.
- **Default to `const auto & [a, b]`**, unless the body mutates or moves through the binding.
- **`itkLabelOverlapMeasuresImageFilter.hxx:52` is the exception** — the body does `m1.emplace(m2_value.first, std::move(m2_value.second))`, so it moves out of the mapped value. It must be `for (auto & [label, measures] : m2)`; `const auto &` will not compile.
- **Watch for `std::map>`** — a nested destructure `[k, [a, b]]` is not valid C++17; flag such sites for human review instead of converting.
- **Add no comments.** The binding names are the explanation.
Suggested PR split
1. **`PERF:` — `Filtering/ImageStatistics`** (3 sites). Lead with the by-value repair at `itkLabelOverlapMeasuresImageFilter.hxx:52`; structured bindings ride along as the readability bonus. Land this first — it is the only site with a behavioural justification.
2. **`STYLE:` — `itkProcessObject.cxx`** (9 sites, one file, one commit). Cleanest single review unit.
3. **`STYLE:` — rest of `Core/Common` + `Core/SpatialObjects`** (6 sites, one commit per file).
4. **`STYLE:` — long tail** (`Segmentation/ConnectedComponents`, `IO/XML`, `Nonunit/Review`, `Core/Mesh`, `Filtering/ImageCompose`, `Examples/IO/XML`, tests) — 8 sites; split further if a reviewer prefers.
Finder script (re-run to refresh the inventory as code evolves)
```python
RANGE_FOR = re.compile(
r'for\s*\(\s*(?:const\s+)?(?:auto\b\s*&?&?\s*'
r'|[\w:<>,\s\*]+&?\s*)(?P[A-Za-z_]\w*)\s*:\s*[^\)]*\)'
)
# For each match, scan the next 15 lines for BOTH
# r'\b' + name + r'\.first\b' AND r'\b' + name + r'\.second\b'
# -> that loop is a structured-binding candidate.
```
Run over `.h/.hxx/.cxx/.txx`, excluding `ThirdParty/`.
Contributor guide
Research direction
Start with Modules/Core/Common/src/itkProcessObject.cxx, which contains nine of the 26 listed sites, then review the remaining files in the inventory. Use the provided finder script to confirm candidates and inspect the by-value loop in itkLabelOverlapMeasuresImageFilter.hxx separately. Done means all suitable sites use meaningful C++17 bindings, the move case remains valid, and the relevant ITK build and tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend, performance
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100