And replaced some static with dynamic casts.
Auto-correction using "clang-tidy -fix ..." plus manual correction of 'const'
qualifier where needed.
Details
Details
- Reviewers
gengisdave - Group Reviewers
Krusader - Commits
- R167:cebdfda551cc: Merge branch 'my-clang-fixes-wrong-cast-fixes'
Tested application start and Disc Usage tool.
Diff Detail
Diff Detail
- Repository
- R167 Krusader
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Hi, Alex, thanks for improving the source code, I would like to add that the krusader/GUI/krhistorycombobox.cpp code has been reviewed recently and replacing
auto keyEvent = static_cast<QKeyEvent *>(event);
with
auto keyEvent = dynamic_cast<QKeyEvent *>(event);
would not be what it's written in https://doc.qt.io/qt-5/qobject.html:
QKeyEvent *keyEvent = static_cast<QKeyEvent *>(event);
The same could be said about
krusader/Panel/krsearchbar.cpp
and its existing
auto *ke = static_cast<QKeyEvent *>(event);
While we are at it, what about using qobject_cast (instead of dynamic_cast) in the following places?
diff --git a/krusader/KViewer/lister.cpp b/krusader/KViewer/lister.cpp --- a/krusader/KViewer/lister.cpp +++ b/krusader/KViewer/lister.cpp @@ -1360,7 +1360,7 @@ bool Lister::openUrl(const QUrl &listerUrl) connect(downloadJob, &KIO::TransferJob::result, this, [=](KJob *job) { _tempFile->flush(); if (job->error()) { /* any error occurred? */ - auto *kioJob = (KIO::TransferJob *)job; + auto *kioJob = qobject_cast<KIO::TransferJob *>(job); KMessageBox::error(_textArea, i18n("Error reading file %1.", kioJob->url().toDisplayString(QUrl::PreferLocalFile))); } _downloading = false; diff --git a/krusader/Konfigurator/kgdependencies.cpp b/krusader/Konfigurator/kgdependencies.cpp --- a/krusader/Konfigurator/kgdependencies.cpp +++ b/krusader/Konfigurator/kgdependencies.cpp @@ -145,7 +145,7 @@ void KgDependencies::addApplication(const QString& name, QGridLayout *grid, int void KgDependencies::slotApply(QObject *obj, const QString& configGroup, const QString& name) { - auto *urlRequester = (KonfiguratorURLRequester *) obj; + auto *urlRequester = qobject_cast<KonfiguratorURLRequester *>( obj); KConfigGroup group(krConfig, configGroup); group.writeEntry(name, urlRequester->url().toDisplayString(QUrl::PreferLocalFile)); diff --git a/krusader/Konfigurator/kggeneral.cpp b/krusader/Konfigurator/kggeneral.cpp --- a/krusader/Konfigurator/kggeneral.cpp +++ b/krusader/Konfigurator/kggeneral.cpp @@ -287,7 +287,7 @@ void KgGeneral::createGeneralTab() void KgGeneral::applyTempDir(QObject *obj, const QString& configGroup, const QString& name) { - auto *urlReq = (KonfiguratorURLRequester *)obj; + auto *urlReq = qobject_cast<KonfiguratorURLRequester *>(obj); QString value = urlReq->url().toDisplayString(QUrl::PreferLocalFile); KConfigGroup(krConfig, configGroup).writeEntry(name, value); ); diff --git a/krusader/Konfigurator/konfigurator.cpp b/krusader/Konfigurator/konfigurator.cpp --- a/krusader/Konfigurator/konfigurator.cpp +++ b/krusader/Konfigurator/konfigurator.cpp @@ -163,7 +163,7 @@ void Konfigurator::closeEvent(QCloseEvent *event) void Konfigurator::slotApplyEnable() { lastPage = currentPage(); - bool isChanged = ((KonfiguratorPage *)(lastPage->widget()))->isChanged(); + bool isChanged = (qobject_cast<KonfiguratorPage *>(lastPage->widget()))->isChanged(); button(QDialogButtonBox::Apply)->setEnabled(isChanged); button(QDialogButtonBox::Reset)->setEnabled(isChanged); } @@ -173,7 +173,7 @@ bool Konfigurator::slotPageSwitch(KPageWidgetItem *current, KPageWidgetItem *bef if (before == nullptr) return true; - auto *currentPg = (KonfiguratorPage *)(before->widget()); + auto *currentPg = qobject_cast<KonfiguratorPage *>(before->widget()); if (internalCall) { internalCall = false; @@ -221,17 +221,17 @@ void Konfigurator::slotClose() void Konfigurator::slotApply() { - emit configChanged(((KonfiguratorPage*)(currentPage()->widget()))->apply()); + emit configChanged((qobject_cast<KonfiguratorPage*>(currentPage()->widget()))->apply()); } void Konfigurator::slotReset() { - ((KonfiguratorPage *)(currentPage()->widget()))->loadInitialValues(); + (qobject_cast<KonfiguratorPage *>(currentPage()->widget()))->loadInitialValues(); } void Konfigurator::slotRestoreDefaults() { - ((KonfiguratorPage *)(currentPage()->widget()))->setDefaults(); + (qobject_cast<KonfiguratorPage *>(currentPage()->widget()))->setDefaults(); } void Konfigurator::slotShowHelp() diff --git a/krusader/Synchronizer/synchronizer.cpp b/krusader/Synchronizer/synchronizer.cpp --- a/krusader/Synchronizer/synchronizer.cpp +++ b/krusader/Synchronizer/synchronizer.cpp @@ -179,7 +179,7 @@ void Synchronizer::compareLoop() if (entry->inherits("CompareTask")) { if (entry->state() == ST_STATE_READY) { - auto *ctentry = (CompareTask *) entry; + auto *ctentry = qobject_cast<CompareTask *>( entry); if (ctentry->isDuplicate()) compareDirectory(ctentry->parent(), ctentry->leftDirList(), ctentry->rightDirList(), ctentry->leftDir(), ctentry->rightDir()); diff --git a/krusader/Synchronizer/synchronizertask.cpp b/krusader/Synchronizer/synchronizertask.cpp --- a/krusader/Synchronizer/synchronizertask.cpp +++ b/krusader/Synchronizer/synchronizertask.cpp @@ -272,8 +272,8 @@ void CompareContentTask::slotDataReceived(KIO::Job *job, const QByteArray &data) if (compareArray.size()) abortContentComparing(); } else { - if (!((KIO::TransferJob *)job)->isSuspended()) { - ((KIO::TransferJob *)job)->suspend(); + if (!(qobject_cast<KIO::TransferJob *>(job))->isSuspended()) { + (qobject_cast<KIO::TransferJob *>(job))->suspend(); otherJob->resume(); } } diff --git a/krusader/actionsbase.cpp b/krusader/actionsbase.cpp --- a/krusader/actionsbase.cpp +++ b/krusader/actionsbase.cpp @@ -98,13 +98,13 @@ QAction *ActionsBase::action(QString text, QString icon, const QKeySequence& sho KToggleAction *ActionsBase::toggleAction(QString text, QString icon, const QKeySequence& shortcut, QObject *recv, const char *slot, QString name) { - return (KToggleAction *)(action(std::move(text), std::move(icon), shortcut, recv, slot, std::move(name), true)); + return qobject_cast<KToggleAction *>(action(std::move(text), std::move(icon), shortcut, recv, slot, std::move(name), true)); } KToggleAction *ActionsBase::toggleAction(QString text, QString icon, const QKeySequence& shortcut, ActionGroup &group, const char *slot, QString name) { - return (KToggleAction *)(action(std::move(text), std::move(icon), shortcut, group, slot, std::move(name), true)); + return qobject_cast<KToggleAction *>(action(std::move(text), std::move(icon), shortcut, group, slot, std::move(name), true)); } QAction *ActionsBase::stdAction(KStandardAction::StandardAction id, QObject *recv, const char *slot) diff --git a/krusader/krusader.cpp b/krusader/krusader.cpp --- a/krusader/krusader.cpp +++ b/krusader/krusader.cpp @@ -540,7 +540,7 @@ void Krusader::stopWait() { } void Krusader::updateUserActions() { - auto *userActionMenu = (KActionMenu *) KrActions::actUserMenu; + auto *userActionMenu = qobject_cast<KActionMenu *>( KrActions::actUserMenu); if (userActionMenu) { userActionMenu->menu()->clear();
Greetings!
Comment Actions
Hi Toni,
would not be what it's written in https://doc.qt.io/qt-5/qobject.html:
I also noticed that static_cast is used in the Qt code examples, e.g. also in https://doc.qt.io/qt-5/eventsandfilters.html. But it is not mentioned why and i don't see a higher reason for. As it compiles and is tested, using dynamic cast should be safe.
While we are at it, what about using qobject_cast (instead of dynamic_cast) in the following places?
IMO using the dynamic cast is a big advantage over using the standard c-style or static cast. Using the qobject cast for all QObjects would be an additional advantage on top of that.
I will first merge this patch before there will be code changes which causes merge conflicts. Please apply you're changes as an additional patch.