WIP: Clazy static analysis fixes — learning exercise for Claude Code skills (DO NOT MERGE)#2
Closed
hjmjohnson wants to merge 32 commits into
Closed
WIP: Clazy static analysis fixes — learning exercise for Claude Code skills (DO NOT MERGE)#2hjmjohnson wants to merge 32 commits into
hjmjohnson wants to merge 32 commits into
Conversation
hjmjohnson
force-pushed
the
apply_clazy_skills
branch
12 times, most recently
from
f6ca138 to
5c0a0c3
Compare
…dFilterChange Qt 6.10 deprecated QSortFilterProxyModel::invalidateFilter() in favor of the beginFilterChange()/endFilterChange() pair. Use #if QT_VERSION guards to select the appropriate API while maintaining Qt5 and Qt6 <6.10 compat. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lity QImage::flipped(Qt::Orientations) was introduced in Qt 6.9. While QImage::mirrored() was briefly annotated as deprecated in Qt 6.9 and then un-deprecated in Qt 6.10 (scheduled for removal from Qt 6.13), the newer flipped() API is clearer in intent and is the recommended replacement going forward. Use #if QT_VERSION >= QT_VERSION_CHECK(6, 9, 0) guards to select the appropriate API while maintaining Qt5 compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
• Requires explicit declaration of the enum in Q_ENUMS(...). • Does not support compile-time introspection via QMetaEnum::fromType<T>(). • Print conversions via qDebug() or QVariant conversions not automatically supported. • Replace all Q_ENUMS(...) with Q_ENUM(...) in your QObject classes. • Make sure the enum is declared immediately before the Q_ENUM macro. • After migration, there is seamless integration into Qt’s meta-object system, better introspection, and compatibility with modern Qt5 (5.5+) and Qt6.
Use std::as_const() or const local variables to ensure range-for loops call the const begin()/end() overloads, avoiding unnecessary COW detach on implicitly shared Qt containers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…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>
hjmjohnson
force-pushed
the
apply_clazy_skills
branch
from
5c0a0c3 to
7242847
Compare
…-void-expression) Two void setter functions used `return void_func()` which compiles because the returned type matches the function's void return type, but is misleading and triggers the returning-void-expression check. Removed the needless return keyword. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ctkWorkflowWidgetStep.cpp: showUserInterfaceComplete() is a public method, not a signal. Remove the erroneous emit keyword; the method internally delegates to d->showUserInterfaceCompleteInternal() which emits the signal. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…zy qenums) Q_ENUMS is deprecated since Qt 5.5. Replace the single Q_ENUMS macro listing three enums with individual Q_ENUM macros placed immediately after each enum definition, as required by Q_ENUM. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y qstring-arg) Replace chained .arg().arg() calls with the multi-arg overload to avoid creating unnecessary temporary QString allocations at each step. For integer arguments (__LINE__, entryIndex), convert explicitly via QString::number() since the multi-arg overload requires const QString&. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Apply clazy-qfileinfo-exists fixes. The static QFileInfo::exists() avoids constructing a temporary QFileInfo object for a simple existence check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add missing include directory for QtTesting headers so that ctkVTKRenderViewEventTranslator and related files can find QtTesting headers without specifying full paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove all VTK 8 CMake configuration from the superbuild: - Remove VTK 8 revision tag (v8.0.1) and version validation - Remove additional_vtk8_cmakevars and VTK8 module options (Module_vtkChartsCore, Module_vtkGUISupportQt, etc.) - Remove VTK8-only ExternalProject cache args (VTK_Group_Qt, VTK_QT_VERSION) that are not used by VTK 9's module system CTK_VTK_VERSION_MAJOR is retained but now only accepts "9". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove all CMake code paths that supported VTK 8 and earlier: - Remove include(VTK_USE_FILE) paths (VTK8 pattern) - Remove manual VTK_LIBRARIES lists for VTK 5/6/7/8 - Remove VERSION_LESS "8.90" guards around QVTKOpenGLNativeWidget detection, export content, and vtk_module_autoinit() - Remove VTK8 Python/X11 library linking blocks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove #if VTK_MAJOR_VERSION >= 9 guards in 6 source files, keeping only the VTK9 API calls (renderWindow(), setRenderWindow(), interactor()) and removing VTK8 fallbacks (GetRenderWindow(), SetRenderWindow()). Remove VTK8 fallback warnings for PBR, Metallic, and Roughness in ctkVTKSurfaceMaterialPropertyWidget. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
hjmjohnson
force-pushed
the
apply_clazy_skills
branch
from
7242847 to
ea2d1d7
Compare
…nnect-by-name) Rename slots in ctkPathListButtonsWidgetPrivate that followed the on_<objectName>_<signal> naming convention. These slots were already explicitly connected via connect() calls, not auto-connected via connectSlotsByName(), but the naming pattern is misleading and fragile since it could trigger unintended auto-connections if the object names in the .ui file ever matched. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…y strict-iterators) Replace begin()/end()/find() with cbegin()/constEnd()/constFind() where the result is assigned to a const_iterator. Mixing mutable and const iterators can silently detach shared containers, causing bugs and unnecessary copies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…irtual-call-ctor) Replace this->setViewport(viewport) with direct d->Viewports[QString()] = viewport in the two constructors that accept a viewport parameter. setViewport() calls onViewportChanged() (virtual), which calls refresh() -> setupLayout(), which eventually reaches the pure virtual viewFromXML(). During construction the vtable *may not be fully built*, so virtual dispatch would call the base-class implementation only — and calling a pure virtual function is undefined behavior. The direct assignment has identical observable behavior during construction because: - The oldViewport cleanup branch in setViewport() is never taken (map is empty) - setupLayout() returns early when no layout has been set (which is always true during construction) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove extra spaces after commas and strip const/& qualifiers from type names inside SIGNAL(), SLOT() macros. Normalized signatures avoid unnecessary string allocations in QObject::connect(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensure QObject-derived classes properly use the `Q_OBJECT` macro for meta-object functionality and include corresponding `.moc` files to support Qt's meta-object compiler. Add missing Q_OBJECT to complete Qt inheritance The Q_OBJECT macro is mandatory for any class that: • Inherits from QObject • Uses signals or slots • Declares properties (Q_PROPERTY) • Needs qobject_cast, metaObject(), or other Qt RTTI features Without it: • Qt’s meta-object compiler (moc) won’t generate necessary code • You may encounter runtime errors or silently broken signals/slots • Clazy and Qt's build system may warn or fail Need to include full class definitions instead of forward definitions. Replace forward declarations that were common in Qt 5, but are now discouraged in Qt 6. Full declarations are required when the Q_OBJECT macro is included as recommended to respect the Qt inheritance expectations.
…y missing-qobject-macro) Add Q_OBJECT macro and corresponding #include "*.moc" at end of each test file for QObject subclasses that were missing it. Classes without Q_OBJECT lack signals/slots, qobject_cast support, and Python scripting. Wrap same-named helpers in anonymous namespaces to prevent duplicate symbol linker errors across translation units. Deliberately skipped: LogMessageThread (shared via #include in 5+ TUs — adding Q_OBJECT would cause duplicate meta-object symbols at link time) and QCTK_DECLARE_TEST macro-generated class (would require macro change in ctkTestApplication.h plus .moc includes in all users). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y ctor-missing-parent-argument) All QObject-derived classes that lacked a constructor accepting a parent argument have been updated: declarations get `QObject* parent = nullptr` (or `QWidget*`/`QState*` as appropriate for the base class), definitions pass `parent` to the base-class constructor in the initializer list. Includes: ctkAbstractJob, ctkAbstractWorker, error-log message handlers, workflow transitions, all DICOM Job/Worker subclasses, ctkDICOMIndexerPrivateWorker, ctkConsoleCompleter, ctkMenuComboBoxInternal, ctkPathListButtonsWidget inner proxy models, and ctkTestApplication. ctkDICOMJob.h is added to WRAP_EXCLUDE in the DICOM Core CMakeLists to prevent the PythonQt generator from attempting to instantiate the abstract class. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…non-pod-global-static) Replace file-scope static ctkLogger instances with Q_GLOBAL_STATIC_WITH_ARGS to avoid running constructors at library load time (56 files). Convert static QString/QStringList globals to function-local statics for lazy initialization. Convert ctkDICOMModalities namespace variables from static-in-header to inline functions returning const references, eliminating per-TU copies. Skipped singleton initializer patterns (CTK_SINGLETON_DECLARE_INITIALIZER) and QScopedPointer holders which are deliberate design choices. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d-global-static) Replace five file-scope QScopedPointer statics with a single Q_GLOBAL_STATIC-managed struct. This avoids non-trivial constructors/destructors running at library load/unload time, deferring initialization to first use instead. Six additional warnings in test files and three singleton initializer patterns (CTK_SINGLETON_DECLARE_INITIALIZER) were investigated and deliberately skipped. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…function-args-by-ref) Change function parameters of non-trivially-copyable types (QString, QStringList, QVariant, QDateTime, QDomElement, QModelIndex, QModelIndexList, QMultiMap, ctkServiceReference, ctkDICOMPersonName, etc.) from pass-by-value to pass-by-const-reference. This avoids unnecessary copy construction and reference count bumps on implicitly shared Qt types. Also fix the PythonQt wrapper generator regex for detecting pure virtual methods: the character class was missing '&', ',', ':', '<', '>' so it could not match through const-ref parameters or template types, causing it to incorrectly generate instantiation code for abstract classes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…azy function-args-by-value) Change function parameters from `const T&` to `T` for trivially-copyable types with sizeof <= 16 bytes: bool, int, unsigned short, enum types (QThread::Priority, QtMsgType, ctkErrorLogLevel::LogLevel, etc.), QFlags types (ctkErrorLogTerminalOutput::TerminalOutputs, etc.), QPoint, QSize, QRect, QDate, QPointF, Qt::Alignment, Qt::TextElideMode, Qt::Orientation, Qt::ScrollBarPolicy, QBoxLayout::Direction, and other small enums/flags. These types are trivially copyable and fit in CPU registers on all platforms, so passing by value is more efficient than by-const-reference. Updated both header declarations and cpp definitions for public/protected methods. Private-only methods (in *Private classes) updated in .cpp only. Signals were not changed. The `ctkDICOMQuery::done(const bool& error)` signal was explicitly left unchanged. Slicer downstream impact: ctkVTKRenderView::lookFromAxis() signature changes from `const ctkAxesWidget::Axis&` to `ctkAxesWidget::Axis`. Any Slicer code calling this method will still compile since passing by value accepts both lvalues and rvalues. No virtual override chains were broken. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add the `emit` keyword before signal calls that were missing it, for readability and to satisfy clazy's incorrect-emit check. Refactored LOG_AND_EMIT_* macros in ctkDICOMQuery.cpp and ctkDICOMRetrieve.cpp so that `emit` is visible at call sites rather than hidden inside the macro. Fixed an actual bug in ctkDICOMServer.cpp where `emit setTrustedEnabled()` was incorrectly emitting the setter name instead of the signal `trustedEnabledChanged()`. CTK_SET_CPP_EMIT macro call sites are false positives (the macro already includes emit) and were left unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hjmjohnson
force-pushed
the
apply_clazy_skills
branch
from
ea2d1d7 to
5c33c7a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Warning
DO NOT MERGE — This pull request is a learning exercise and demonstration. It is not ready for upstream submission to commontk/CTK. See notes below on how to prepare individual commits for upstream review.
Purpose
This branch was created as a learning exercise to develop and validate a set of Claude Code AI-assisted skills for systematically improving CTK code quality. The skills are published at:
The goal is to demonstrate how AI-assisted tooling can be used to find and fix Qt code quality issues at scale — issues that affect correctness, performance, and Qt6 portability across CTK and its downstream consumers (most importantly 3D Slicer).
What This Branch Fixes
All fixes were identified by clazy, KDE's Qt-aware static analysis tool. Each commit addresses one check category:
8bb2cde1.mocincludes (AutoMoc hygiene)83c4b8d1QTemporaryFile::open()return valuee35108eduse-static-qregularexpressionafcef0f7global-const-char-pointerb02ff877non-pod-global-staticc1b1877fmissing-qobject-macro636de7cdconst-signal-or-slot59eb9695detaching-temporary1a7c9c85range-loop-detach1a9d95b6returning-void-expression10bc5c6cfunction-args-by-refThe
returning-void-expressioncommit (1a9d95b6) is the most significant: it found real bugs where signals were declared to return values and thereturn signal()pattern silently prevented the signal from ever being emitted.Philosophies Enforced
These principles were applied throughout to minimize future developer effort:
1. One commit per check — linear history
Each clazy check category is a single, self-contained commit. This makes it easy to:
No merge commits. Rebase-only.
2. Commits ordered by impact (safest first)
Commits are ordered from lowest to highest downstream impact:
function-args-by-refcommit, which changes public virtual method signatures and requires coordinated updates in Slicer (e.g.qMRMLLayoutManager::viewFromXML())This ordering allows downstream projects to cherry-pick early commits immediately while the high-impact commit is reviewed separately.
3. Every commit builds cleanly with zero new warnings
Each commit was verified with an incremental build. No commit introduces a compiler warning; the one pre-existing
[-Wunused-result]warning is removed by commit83c4b8d1.4.
PERF:prefix for performance fixes, notSTYLE:Fixes that eliminate unnecessary copies of Qt implicitly-shared types (QString, QStringList, QVariant, etc.) are prefixed
PERF:rather thanSTYLE:. These changes reduce atomic reference-count operations and improve performance — the distinction matters for downstream projects deciding which commits to prioritize.5.
std::as_const()overqAsConst()CTK builds with C++17. All range-for loop fixes use
std::as_const()rather thanqAsConst(), which is deprecated in Qt 6.6+.6. Signal parameters left as pass-by-value
Clazy's
function-args-by-refcheck flags signal parameters, but signal signatures must match their normalizedSIGNAL()macro strings. Signal parameters were deliberately left as pass-by-value throughout.7. PythonQt wrapper generator is sensitive to
const&in virtual signaturesThe
CMake/ctkWrapPythonQt.pyabstract-class detection regex did not handleconst¶meters. This was fixed in the same commit asfunction-args-by-ref— without this fix, the PythonQt wrapper generator would try to instantiate abstract classes and fail to compile.How to Prepare Commits for Upstream (commontk/CTK)
The commits in this branch are ready to submit individually to commontk/CTK. Recommended approach:
1a9d95b6) as a single PR — they are low-risk and have no Slicer API impact.10bc5c6c(function-args-by-ref) as a separate PR with a note to the Slicer team, since it requires updating virtual overrides inqMRMLLayoutManagerand related classes.Claude Code Skills Repo
The CTK-claude-skills repository contains the skill definitions used to produce this branch:
/clazy-configure/clazy-build/clazy-checkclazy-standaloneat level0/1/2, produce prioritized fix list/clazy-fix/clazy-testThese skills encode the domain knowledge needed to apply clazy fixes correctly in the CTK/Slicer context — including the PythonQt wrapper pitfall, the superbuild vs inner-build distinction, and Slicer's downstream virtual method dependencies.