InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

STYLE: Use C++17 structured bindings for map/pair iteration

Open
#6,733 0 comments 1 reaction 0 assignees View on GitHub
area:Core type:Design type:Enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.