Skip to content

BUG: Fix iterator invalidation in ctkCmdLineModuleCache::clearCache - #1396

Merged
lassoan merged 1 commit into
commontk:masterfrom
BRAINSia:fix-clearCache-iterator-invalidation
Mar 30, 2026
Merged

BUG: Fix iterator invalidation in ctkCmdLineModuleCache::clearCache#1396
lassoan merged 1 commit into
commontk:masterfrom
BRAINSia:fix-clearCache-iterator-invalidation

Conversation

@hjmjohnson

Copy link
Copy Markdown
Contributor

Summary

  • Fix undefined behavior introduced by the foreach-to-for conversion in PERF/STYLE/BUG: Container fixes, signal cleanup, and Q_OBJECT macros (clazy) #1383
  • removeCacheEntry() calls d->LocationToXmlDescription.remove(), which invalidates the iterators used by the enclosing for loop
  • The original foreach was safe because Qt's foreach macro copies the container before iterating
  • Restore that safety by iterating over a copy of the keys via .keys()

The clazy container-anti-pattern warning is a false positive here — the copy is required for correctness because the loop body mutates the container.

Test plan

🤖 Generated with Claude Code

The foreach-to-for conversion in commontk#1383 introduced undefined behavior:
removeCacheEntry() calls d->LocationToXmlDescription.remove(), which
invalidates the iterators used by the enclosing for loop.

The original foreach was safe because Qt's foreach macro copies the
container before iterating. Restore that safety by iterating over a
copy of the keys. The clazy container-anti-pattern warning is a false
positive here — the copy is required for correctness.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jamesobutler
jamesobutler requested a review from lassoan March 30, 2026 17:38

@lassoan lassoan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, it looks good to me.

@lassoan
lassoan enabled auto-merge (rebase) March 30, 2026 19:18
@lassoan
lassoan merged commit fec36ce into commontk:master Mar 30, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants