Skip to content

PERF/STYLE/BUG: Container fixes, signal cleanup, and Q_OBJECT macros (clazy) #1383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jamesobutler merged 6 commits into from
Mar 29, 2026
Merged

PERF/STYLE/BUG: Container fixes, signal cleanup, and Q_OBJECT macros (clazy) #1383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
174c048
PERF: Replace QMap with QHash for pointer keys (clazy qmap-with-point…
hjmjohnson Mar 21, 2026
6d198d1
STYLE: Replace container anti-patterns with direct iteration (clazy)
hjmjohnson Mar 20, 2026
2d9444c
PERF: Replace non-const calls on temporaries with const alternatives …
hjmjohnson Mar 22, 2026
208a512
STYLE: Remove const from signals and move getters out of slots sectio…
hjmjohnson Mar 22, 2026
fad125f
BUG: Check QFile::open() return values instead of ignoring them
hjmjohnson Mar 20, 2026
de36049
STYLE: Add missing Q_OBJECT macro to QAbstractItemModel subclasses (c…
hjmjohnson Mar 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Applications/ctkPluginBrowser/ctkQtResourcesTreeModel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class ctkQtResourceTreeItem;

class ctkQtResourcesTreeModel : public QAbstractItemModel
{
Q_OBJECT
public:

ctkQtResourcesTreeModel(QObject* parent = 0);
Expand Down
4 changes: 2 additions & 2 deletions Libs/CommandLineModules/Core/ctkCmdLineModuleCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ void ctkCmdLineModuleCache::removeCacheEntry(const QUrl& moduleLocation)

void ctkCmdLineModuleCache::clearCache()
{
foreach(const QUrl &url, d->LocationToXmlDescription.keys())
for (auto it = d->LocationToXmlDescription.constBegin(); it != d->LocationToXmlDescription.constEnd(); ++it)
{
removeCacheEntry(url);
removeCacheEntry(it.key());

@lassoan lassoan Mar 29, 2026

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.

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.

@hjmjohnson hjmjohnson Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@hjmjohnson hjmjohnson Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lassoan You are correct! This is a potential bug. I'm working on a fix.

@hjmjohnson hjmjohnson Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

}
}
6 changes: 3 additions & 3 deletions Libs/Core/Testing/Cpp/ctkExampleWorkflowStepUsingSignalsAndSlots.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ protected Q_SLOTS:
///
/// Signals indicating to the workflow that these processes have
/// completed
void validationComplete(bool validationSucceeded, const QString& branchId = "")const;
void onEntryComplete()const;
void onExitComplete()const;
void validationComplete(bool validationSucceeded, const QString& branchId = "");
void onEntryComplete();
void onExitComplete();

protected:
QScopedPointer<ctkExampleWorkflowStepUsingSignalsAndSlotsPrivate> d_ptr;
Expand Down
6 changes: 5 additions & 1 deletion Libs/Core/Testing/Cpp/ctkUtilsCopyDirRecursivelyTest1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ bool createFile(int line, const QDir& dir, const QString& relativePath, const QS
newDir.cd(relativePath);
QString filePath = QFileInfo(newDir, fileName).filePath();
QFile file(filePath);
file.open(QIODevice::Text | QIODevice::WriteOnly);
if (!file.open(QIODevice::Text | QIODevice::WriteOnly))
{
std::cerr << "Failed to open file for writing: " << qPrintable(filePath) << std::endl;
return EXIT_FAILURE;
}
QTextStream out(&file);
out << "Generated by ctkUtilsCopyDirRecursivelyTest1" << ctk::endl;
file.close();
Expand Down
6 changes: 5 additions & 1 deletion Libs/Core/Testing/Cpp/ctkUtilsIsDirEmptyTest1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ int ctkUtilsIsDirEmptyTest1(int argc, char * argv [] )
// Create file
QString filePath = tempDir.filePath("file.txt");
QFile file(filePath);
file.open(QIODevice::Text | QIODevice::WriteOnly);
if (!file.open(QIODevice::Text | QIODevice::WriteOnly))
{
qWarning() << "Failed to open file for writing:" << filePath;
return EXIT_FAILURE;
}
QTextStream out(&file);
out << "Generated by ctkUtilsIsDirEmptyTest1" << ctk::endl;
file.close();
Expand Down
6 changes: 5 additions & 1 deletion Libs/Core/Testing/Cpp/ctkUtilsTest4.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ int createFile(int line, const QDir& dir, const QString& relativePath, const QSt
newDir.cd(relativePath);
QString filePath = QFileInfo(newDir, fileName).filePath();
QFile file(filePath);
file.open(QIODevice::Text | QIODevice::WriteOnly);
if (!file.open(QIODevice::Text | QIODevice::WriteOnly))
{
std::cerr << "Failed to open file for writing: " << qPrintable(filePath) << std::endl;
return EXIT_FAILURE;
}
QTextStream out(&file);
out << "Generated by ctkUtilsTest4" << ctk::endl;
file.close();
Expand Down
8 changes: 4 additions & 4 deletions Libs/Core/Testing/Cpp/ctkWorkflowTest1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,9 @@ int ctkWorkflowTest1(int argc, char * argv [] )
}

if (workflow->forwardSteps(step1).length() != 1
|| workflow->forwardSteps(step1).first() != step2
|| workflow->forwardSteps(step1).constFirst() != step2
|| workflow->forwardSteps(step2).length() != 1
|| workflow->forwardSteps(step2).first() != step3
|| workflow->forwardSteps(step2).constFirst() != step3
|| workflow->forwardSteps(step3).length() != 0)
{
std::cerr << "error in list of forward steps" << std::endl;
Expand All @@ -316,9 +316,9 @@ int ctkWorkflowTest1(int argc, char * argv [] )

if (workflow->backwardSteps(step1).length() != 0
|| workflow->backwardSteps(step2).length() != 1
|| workflow->backwardSteps(step2).first() != step1
|| workflow->backwardSteps(step2).constFirst() != step1
|| workflow->backwardSteps(step3).length() != 1
|| workflow->backwardSteps(step3).first() != step2)
|| workflow->backwardSteps(step3).constFirst() != step2)
{
std::cerr << "error in list of backward steps" << std::endl;
return EXIT_FAILURE;
Expand Down
28 changes: 13 additions & 15 deletions Libs/Core/ctkErrorLogAbstractModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,9 @@ ctkErrorLogAbstractModelPrivate::ctkErrorLogAbstractModelPrivate(ctkErrorLogAbst
// --------------------------------------------------------------------------
ctkErrorLogAbstractModelPrivate::~ctkErrorLogAbstractModelPrivate()
{
foreach(const QString& handlerName, this->RegisteredHandlers.keys())
for (auto it = this->RegisteredHandlers.constBegin(); it != this->RegisteredHandlers.constEnd(); ++it)
{
ctkErrorLogAbstractMessageHandler * msgHandler =
this->RegisteredHandlers.value(handlerName);
ctkErrorLogAbstractMessageHandler * msgHandler = it.value();
Q_ASSERT(msgHandler);
msgHandler->setEnabled(false);
delete msgHandler;
Expand Down Expand Up @@ -162,7 +161,7 @@ bool ctkErrorLogAbstractModel::registerMsgHandler(ctkErrorLogAbstractMessageHand
{
return false;
}
if (d->RegisteredHandlers.keys().contains(msgHandler->handlerName()))
if (d->RegisteredHandlers.contains(msgHandler->handlerName()))
{
return false;
}
Expand All @@ -187,7 +186,7 @@ QStringList ctkErrorLogAbstractModel::msgHandlerNames()const
bool ctkErrorLogAbstractModel::msgHandlerEnabled(const QString& handlerName) const
{
Q_D(const ctkErrorLogAbstractModel);
if (!d->RegisteredHandlers.keys().contains(handlerName))
if (!d->RegisteredHandlers.contains(handlerName))
{
return false;
}
Expand All @@ -198,7 +197,7 @@ bool ctkErrorLogAbstractModel::msgHandlerEnabled(const QString& handlerName) con
void ctkErrorLogAbstractModel::setMsgHandlerEnabled(const QString& handlerName, bool enabled)
{
Q_D(ctkErrorLogAbstractModel);
if (!d->RegisteredHandlers.keys().contains(handlerName))
if (!d->RegisteredHandlers.contains(handlerName))
{
// qCritical() << "Failed to enable/disable message handler " << handlerName
// << "- Handler not registered !";
Expand All @@ -212,11 +211,11 @@ QStringList ctkErrorLogAbstractModel::msgHandlerEnabled() const
{
Q_D(const ctkErrorLogAbstractModel);
QStringList msgHandlers;
foreach(const QString& handlerName, d->RegisteredHandlers.keys())
for (auto it = d->RegisteredHandlers.constBegin(); it != d->RegisteredHandlers.constEnd(); ++it)
{
if (d->RegisteredHandlers.value(handlerName)->enabled())
if (it.value()->enabled())
{
msgHandlers << handlerName;
msgHandlers << it.key();
}
}
return msgHandlers;
Expand Down Expand Up @@ -247,9 +246,9 @@ void ctkErrorLogAbstractModel::disableAllMsgHandler()
void ctkErrorLogAbstractModel::setAllMsgHandlerEnabled(bool enabled)
{
Q_D(ctkErrorLogAbstractModel);
foreach(const QString& msgHandlerName, d->RegisteredHandlers.keys())
for (auto it = d->RegisteredHandlers.constBegin(); it != d->RegisteredHandlers.constEnd(); ++it)
{
this->setMsgHandlerEnabled(msgHandlerName, enabled);
this->setMsgHandlerEnabled(it.key(), enabled);
}
}

Expand Down Expand Up @@ -478,10 +477,9 @@ void ctkErrorLogAbstractModel::setAsynchronousLogging(bool value)
return;
}

foreach(const QString& handlerName, d->RegisteredHandlers.keys())
for (auto it = d->RegisteredHandlers.constBegin(); it != d->RegisteredHandlers.constEnd(); ++it)
{
d->setMessageHandlerConnection(
d->RegisteredHandlers.value(handlerName), value);
d->setMessageHandlerConnection(it.value(), value);
}

QObject::disconnect(this,
Expand Down Expand Up @@ -582,7 +580,7 @@ int ctkErrorLogAbstractModel::logEntryCount()const
ctkErrorLogAbstractMessageHandler* ctkErrorLogAbstractModel::msgHandler(const QString& handlerName)const
{
Q_D(const ctkErrorLogAbstractModel);
if (!d->RegisteredHandlers.keys().contains(handlerName))
if (!d->RegisteredHandlers.contains(handlerName))
{
return nullptr;
}
Expand Down
14 changes: 7 additions & 7 deletions Libs/Core/ctkWorkflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,12 +296,12 @@ void ctkWorkflowPrivate::createTransitionToPreviousStartingStep(ctkWorkflowStep*
// --------------------------------------------------------------------------
ctkWorkflowStep* ctkWorkflowPrivate::stepFromId(const QString& id)const
{
foreach(ctkWorkflowStep* step, this->StepToForwardAndBackwardStepMap.keys())
for (auto it = this->StepToForwardAndBackwardStepMap.constBegin(); it != this->StepToForwardAndBackwardStepMap.constEnd(); ++it)
{
Q_ASSERT(step);
if (QString::compare(step->id(), id, Qt::CaseInsensitive) == 0)
Q_ASSERT(it.key());
if (QString::compare(it.key()->id(), id, Qt::CaseInsensitive) == 0)
{
return step;
return it.key();
}
}
return 0;
Expand Down Expand Up @@ -739,11 +739,11 @@ QList<ctkWorkflowStep*> ctkWorkflow::finishSteps()const

// iterate through our list of steps, and keep the steps that don't have anything following them
QList<ctkWorkflowStep*> finishSteps;
foreach (ctkWorkflowStep* step, d->StepToForwardAndBackwardStepMap.keys())
for (auto it = d->StepToForwardAndBackwardStepMap.constBegin(); it != d->StepToForwardAndBackwardStepMap.constEnd(); ++it)
{
if (!this->canGoForward(step))
if (!this->canGoForward(it.key()))
{
finishSteps.append(step);
finishSteps.append(it.key());
}
}
return finishSteps;
Expand Down
36 changes: 18 additions & 18 deletions Libs/Core/ctkWorkflowStep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,37 +82,37 @@ ctkWorkflowStepPrivate::~ctkWorkflowStepPrivate()
}

// --------------------------------------------------------------------------
void ctkWorkflowStepPrivate::validationCompleteInternal(bool validationResults, const QString& branchId)const
void ctkWorkflowStepPrivate::validationCompleteInternal(bool validationResults, const QString& branchId)
{
emit validationComplete(validationResults, branchId);
}

// --------------------------------------------------------------------------
void ctkWorkflowStepPrivate::onEntryCompleteInternal()const
void ctkWorkflowStepPrivate::onEntryCompleteInternal()
{
emit onEntryComplete();
}

// --------------------------------------------------------------------------
void ctkWorkflowStepPrivate::onExitCompleteInternal()const
void ctkWorkflowStepPrivate::onExitCompleteInternal()
{
emit onExitComplete();
}

// --------------------------------------------------------------------------
void ctkWorkflowStepPrivate::invokeValidateCommandInternal(const QString& desiredBranchId)const
void ctkWorkflowStepPrivate::invokeValidateCommandInternal(const QString& desiredBranchId)
{
emit invokeValidateCommand(desiredBranchId);
}

// --------------------------------------------------------------------------
void ctkWorkflowStepPrivate::invokeOnEntryCommandInternal(const ctkWorkflowStep* comingFrom, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)const
void ctkWorkflowStepPrivate::invokeOnEntryCommandInternal(const ctkWorkflowStep* comingFrom, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)
{
emit invokeOnEntryCommand(comingFrom, transitionType);
}

// --------------------------------------------------------------------------
void ctkWorkflowStepPrivate::invokeOnExitCommandInternal(const ctkWorkflowStep* goingTo, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)const
void ctkWorkflowStepPrivate::invokeOnExitCommandInternal(const ctkWorkflowStep* goingTo, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)
{
emit invokeOnExitCommand(goingTo, transitionType);
}
Expand Down Expand Up @@ -210,44 +210,44 @@ QObject* ctkWorkflowStep::ctkWorkflowStepQObject()
}

// --------------------------------------------------------------------------
void ctkWorkflowStep::validationComplete(bool validationResults, const QString& branchId)const
void ctkWorkflowStep::validationComplete(bool validationResults, const QString& branchId)
{
Q_D(const ctkWorkflowStep);
Q_D(ctkWorkflowStep);
d->validationCompleteInternal(validationResults, branchId);
}

// --------------------------------------------------------------------------
void ctkWorkflowStep::onEntryComplete()const
void ctkWorkflowStep::onEntryComplete()
{
Q_D(const ctkWorkflowStep);
Q_D(ctkWorkflowStep);
d->onEntryCompleteInternal();
}

// --------------------------------------------------------------------------
void ctkWorkflowStep::onExitComplete()const
void ctkWorkflowStep::onExitComplete()
{
Q_D(const ctkWorkflowStep);
Q_D(ctkWorkflowStep);
d->onExitCompleteInternal();
}

// --------------------------------------------------------------------------
void ctkWorkflowStep::invokeValidateCommand(const QString& desiredBranchId)const
void ctkWorkflowStep::invokeValidateCommand(const QString& desiredBranchId)
{
Q_D(const ctkWorkflowStep);
Q_D(ctkWorkflowStep);
d->invokeValidateCommandInternal(desiredBranchId);
}

// --------------------------------------------------------------------------
void ctkWorkflowStep::invokeOnEntryCommand(const ctkWorkflowStep* comingFrom, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)const
void ctkWorkflowStep::invokeOnEntryCommand(const ctkWorkflowStep* comingFrom, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)
{
Q_D(const ctkWorkflowStep);
Q_D(ctkWorkflowStep);
d->invokeOnEntryCommandInternal(comingFrom, transitionType);
}

// --------------------------------------------------------------------------
void ctkWorkflowStep::invokeOnExitCommand(const ctkWorkflowStep* goingTo, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)const
void ctkWorkflowStep::invokeOnExitCommand(const ctkWorkflowStep* goingTo, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)
{
Q_D(const ctkWorkflowStep);
Q_D(ctkWorkflowStep);
d->invokeOnExitCommandInternal(goingTo, transitionType);
}

Expand Down
12 changes: 6 additions & 6 deletions Libs/Core/ctkWorkflowStep.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,37 +208,37 @@ class CTK_CORE_EXPORT ctkWorkflowStep
/// step's processing should be performed.
///
/// \sa validation()
void invokeValidateCommand(const QString& desiredBranchId = QString())const;
void invokeValidateCommand(const QString& desiredBranchId = QString());

/// \brief Signal (emitted by the private implementation) indicating that validation of this
/// step's processing has completed.
///
/// \sa validation()
void validationComplete(bool validationSuceeded, const QString& branchId = QString())const;
void validationComplete(bool validationSuceeded, const QString& branchId = QString());

/// \brief Signal (emitted by the private implementation) indicating that the step's 'onEntry'
/// processing should be performed.
///
/// \sa onEntry()
void invokeOnEntryCommand(const ctkWorkflowStep* comingFrom, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)const;
void invokeOnEntryCommand(const ctkWorkflowStep* comingFrom, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType);

/// \brief Signal (emitted by the private implementation) indicating that the step's 'onEntry'
/// processing has completed.
///
/// \sa onEntry()
void onEntryComplete()const;
void onEntryComplete();

/// \brief Signal (emitted by the private implementation) indicating that the step's 'onExit'
/// processing should be performed.
///
/// \sa onExit()
void invokeOnExitCommand(const ctkWorkflowStep* goingTo, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType)const;
void invokeOnExitCommand(const ctkWorkflowStep* goingTo, const ctkWorkflowInterstepTransition::InterstepTransitionType transitionType);

/// \brief Signal (emitted by the private implementation) indicating that the step's 'onExit'
/// processing has completed.
///
/// \sa onExit()
void onExitComplete()const;
void onExitComplete();

protected:
QScopedPointer<ctkWorkflowStepPrivate> d_ptr;
Expand Down
Loading
Loading