PERF/STYLE/BUG: Container fixes, signal cleanup, and Q_OBJECT macros (clazy) - #1383
Conversation
7bd508b to
f670cc8
Compare
…er-key) QMap sorts by key, but sorting by memory address is meaningless and non-deterministic. Replace QMap<Pointer*, V> with QHash<Pointer*, V> for faster O(1) lookups without spurious ordering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace foreach(x, map.keys()) and foreach(x, map.values()) with constBegin/constEnd iterator loops to avoid creating temporary QList copies. Also replace map.keys().contains() with map.contains(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…(clazy detaching-temporary) Replace .first()/.front() with .constFirst() and operator[] with .at() when called on temporary containers (function return values) to avoid unnecessary implicit detach of shared data. While the refcount is typically 1 for temporaries, using const accessors is safer and avoids potential deep copies if the code is later refactored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ns (clazy const-signal-or-slot) Remove the const qualifier from signal declarations in workflow step private classes (ctkWorkflowStepPrivate, ctkWorkflowWidgetStepPrivate) and their public forwarding methods, since signals imply state change and should not be const. Also propagate the const removal to the *Internal() wrapper methods that emit these signals. Move const getter methods that were incorrectly placed in public/protected slots sections to the appropriate public/protected sections in ctkAxesWidget, ctkDoubleSpinBox, ctkVTKAbstractView, ctkVTKVolumePropertyWidget, and ctkProxyStyle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Qt6 marks QFile::open() as [[nodiscard]]. Rather than suppressing the warning, check the return value and handle failures: - Production code: log warning and return early or throw exception - Test code: report error and return EXIT_FAILURE - ctkDICOMDatabase: use open() return directly instead of separate isOpen() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lazy missing-qobject-macro) Add Q_OBJECT to ctkQtResourcesTreeModel and ctkDICOMItemTreeModel. Without it, qobject_cast<> and metaObject()->className() return incorrect results for these public API classes. 17 additional warnings in test helper classes were investigated and found to be intentional omissions (no signals/slots, adding Q_OBJECT would require .moc includes for no benefit). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f670cc8 to
de36049
Compare
lassoan
left a comment
lassoan
left a comment
There was a problem hiding this comment.
Thanks for working on these. All the changes look good to me (it was sad to see that we had to introduce more complex syntax to improve performance, but it is probably worth it). There was just one change that might cause problems - I added an inline comment.
| for (auto it = d->LocationToXmlDescription.constBegin(); it != d->LocationToXmlDescription.constEnd(); ++it) | ||
| { | ||
| removeCacheEntry(url); | ||
| removeCacheEntry(it.key()); |
There was a problem hiding this comment.
Has this been tested? removeCacheEntry calls d->LocationToXmlDescription.remove(moduleLocation), which may lead to undefined behavior. To me it looks like that making a copy of the container is necessary here.
There was a problem hiding this comment.
I have test on against both Qt5 and Qt6, and built Slicer against this code base with no build errors or test regressions. Let me do some investigating.
There was a problem hiding this comment.
@lassoan You are correct! This is a potential bug. I'm working on a fix.
There was a problem hiding this comment.
@lassoan Good catch — confirmed this is indeed undefined behavior. removeCacheEntry() calls d->LocationToXmlDescription.remove(), which invalidates the live iterators from constBegin()/constEnd().
The original foreach was safe because Qt's foreach macro implicitly copies the container before iterating. The conversion to a direct iterator loop lost that safety.
I audited all ~52 other foreach-to-for conversions in this PR and this is the only case where the loop body mutates the iterated container.
Fix submitted in #1396 — iterates over a copy of the keys via .keys(), which restores the original safety. The clazy container-anti-pattern warning is a false positive here since the copy is required for correctness.
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>
The foreach-to-for conversion in #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>
3 participants
Summary
Part 2 of the clazy static analysis fix series (see #1372 for the full set).
Stacks on #1382.
QMap<Pointer*, V>withQHash<Pointer*, V>for pointer keys (clazyqmap-with-pointer-key).values(),.keys(),.toList()) with direct iteration (clazycontainer-anti-pattern)detaching-temporary)constqualifier from signals/slots, move getters out of slots sections (clazyconst-signal-or-slot)QFile::open()return values instead of ignoring themQ_OBJECTmacro toQAbstractItemModelsubclasses (clazymissing-qobject-macro)Test plan
Part of #1372.
🤖 Generated with Claude Code