D19913: [plasma-desktop] make it compiles without foreach
mlaurent updated this revision to Diff 80170. mlaurent added a comment. Fix David comment REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19913?vs=80169=80170 BRANCH compile_without_foreach (branched from master) REVISION DETAIL https://phabricator.kde.org/D19913 AFFECTED FILES CMakeLists.txt autotests/coronatest.cpp autotests/utils.h src/declarativeimports/calendar/daysmodel.cpp src/declarativeimports/calendar/eventpluginsmanager.cpp src/declarativeimports/core/datamodel.cpp src/declarativeimports/core/datamodel.h src/declarativeimports/core/datasource.cpp src/declarativeimports/core/iconitem.cpp src/declarativeimports/core/tooltipdialog.cpp src/declarativeimports/plasmacomponents/qmenu.cpp src/declarativeimports/plasmaextracomponents/fallbackcomponent.cpp src/plasma/containment.cpp src/plasma/corona.cpp src/plasma/datacontainer.cpp src/plasma/dataengine.cpp src/plasma/dataengineconsumer.cpp src/plasma/package.cpp src/plasma/pluginloader.cpp src/plasma/private/applet_p.cpp src/plasma/private/containment_p.cpp src/plasma/private/dataenginemanager.cpp src/plasma/private/theme_p.cpp src/plasma/private/timetracker.cpp src/plasma/service.cpp src/plasma/svg.cpp src/plasmaquick/appletquickitem.cpp src/plasmaquick/configview.cpp src/plasmaquick/dialog.cpp src/plasmaquick/dialogshadows.cpp src/scriptengines/qml/plasmoid/appletinterface.cpp src/scriptengines/qml/plasmoid/containmentinterface.cpp src/scriptengines/qml/plasmoid/dropmenu.cpp tests/kplugins/plugintest.cpp To: mlaurent, dfaure Cc: nicolasfella, broulik, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
mlaurent updated this revision to Diff 80169. mlaurent added a comment. Port last element to for(...:...) REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19913?vs=80168=80169 BRANCH compile_without_foreach (branched from master) REVISION DETAIL https://phabricator.kde.org/D19913 AFFECTED FILES CMakeLists.txt autotests/coronatest.cpp autotests/utils.h src/declarativeimports/calendar/daysmodel.cpp src/declarativeimports/calendar/eventpluginsmanager.cpp src/declarativeimports/core/datamodel.cpp src/declarativeimports/core/datamodel.h src/declarativeimports/core/datasource.cpp src/declarativeimports/core/iconitem.cpp src/declarativeimports/core/tooltipdialog.cpp src/declarativeimports/plasmacomponents/qmenu.cpp src/declarativeimports/plasmaextracomponents/fallbackcomponent.cpp src/plasma/containment.cpp src/plasma/corona.cpp src/plasma/datacontainer.cpp src/plasma/dataengine.cpp src/plasma/dataengineconsumer.cpp src/plasma/package.cpp src/plasma/pluginloader.cpp src/plasma/private/applet_p.cpp src/plasma/private/containment_p.cpp src/plasma/private/dataenginemanager.cpp src/plasma/private/theme_p.cpp src/plasma/private/timetracker.cpp src/plasma/service.cpp src/plasma/svg.cpp src/plasmaquick/appletquickitem.cpp src/plasmaquick/configview.cpp src/plasmaquick/dialog.cpp src/plasmaquick/dialogshadows.cpp src/scriptengines/qml/plasmoid/appletinterface.cpp src/scriptengines/qml/plasmoid/containmentinterface.cpp src/scriptengines/qml/plasmoid/dropmenu.cpp tests/kplugins/plugintest.cpp To: mlaurent, dfaure Cc: nicolasfella, broulik, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
mlaurent updated this revision to Diff 80168. mlaurent added a comment. Rebase against master REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19913?vs=55796=80168 BRANCH compile_without_foreach (branched from master) REVISION DETAIL https://phabricator.kde.org/D19913 AFFECTED FILES CMakeLists.txt autotests/coronatest.cpp src/declarativeimports/calendar/daysmodel.cpp src/declarativeimports/calendar/eventpluginsmanager.cpp src/declarativeimports/core/datamodel.cpp src/declarativeimports/core/datamodel.h src/declarativeimports/core/datasource.cpp src/declarativeimports/core/iconitem.cpp src/declarativeimports/core/tooltipdialog.cpp src/declarativeimports/plasmacomponents/qmenu.cpp src/declarativeimports/plasmaextracomponents/fallbackcomponent.cpp src/plasma/containment.cpp src/plasma/corona.cpp src/plasma/datacontainer.cpp src/plasma/dataengine.cpp src/plasma/dataengineconsumer.cpp src/plasma/package.cpp src/plasma/pluginloader.cpp src/plasma/private/applet_p.cpp src/plasma/private/containment_p.cpp src/plasma/private/dataenginemanager.cpp src/plasma/private/theme_p.cpp src/plasma/private/timetracker.cpp src/plasma/service.cpp src/plasma/svg.cpp src/plasmaquick/appletquickitem.cpp src/plasmaquick/configview.cpp src/plasmaquick/dialog.cpp src/plasmaquick/dialogshadows.cpp src/scriptengines/qml/plasmoid/appletinterface.cpp src/scriptengines/qml/plasmoid/containmentinterface.cpp tests/kplugins/plugintest.cpp To: mlaurent, dfaure Cc: nicolasfella, broulik, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
nicolasfella added a comment. The title says plasma-desktop, but this is plasma-framework? REPOSITORY R242 Plasma Framework (Library) BRANCH compile_without_foreach (branched from master) REVISION DETAIL https://phabricator.kde.org/D19913 To: mlaurent, dfaure Cc: nicolasfella, broulik, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.
Looks like this got lost/abandoned? It's painful to review (because it's so
long), but it shouldn't be lost work...
Can you rebase and see if it still applies?
Maybe in the future better split this up into multiple patches so it can land
in chunks instead of getting stuck forever
INLINE COMMENTS
> broulik wrote in daysmodel.cpp:171
> While at it `const QDate &`
Actually better not, a QDate is just a wrapper for a qint64, with a generated
copy constructor (so it's just "copying" a qint64)
> corona.cpp:467
> KConfigGroup containmentsGroup(cg, "Containments");
> -foreach (const Containment *containment, containments) {
> +for (const Containment *containment : qAsConst(containments)) {
> QString cid = QString::number(containment->id());
(already const, this method is const)
> apol wrote in pluginloader.cpp:567
> shouldn't it be const?
(it is now)
REPOSITORY
R242 Plasma Framework (Library)
BRANCH
compile_without_foreach (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D19913
To: mlaurent, dfaure
Cc: broulik, apol, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham,
bruns
D19913: [plasma-desktop] make it compiles without foreach
mlaurent added inline comments. INLINE COMMENTS > broulik wrote in coronatest.cpp:216 > why not make the container `const`? Because this variable is use in code see line 228 REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D19913 To: mlaurent, dfaure Cc: broulik, apol, kde-frameworks-devel, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
broulik added inline comments.
INLINE COMMENTS
> coronatest.cpp:150
> +const auto containments = m_corona->containments();
> +for (auto cont : containments) {
> switch (cont->id()) {
Please always annotate `auto` with e.g. asterisk or ampersand depending on type
> coronatest.cpp:216
>
> -foreach (Plasma::Containment *cont, m_corona->containments()) {
> +auto containments = m_corona->containments();
> +for (Plasma::Containment *cont : qAsConst(containments)) {
why not make the container `const`?
> daysmodel.cpp:171
>
> -Q_FOREACH (const QDate date, updatesList) {
> +for (const QDate date : qAsConst(updatesList)) {
> const QModelIndex changedIndex = indexForDate(date);
While at it `const QDate &`
> apol wrote in datamodel.cpp:309
> same?
> Also this should clearly be using iterators, no?
Yes, please change it to iterators
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
https://phabricator.kde.org/D19913
To: mlaurent, dfaure
Cc: broulik, apol, kde-frameworks-devel, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
mlaurent updated this revision to Diff 55796. mlaurent added a comment. Rename some variables REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19913?vs=54920=55796 BRANCH compile_without_foreach (branched from master) REVISION DETAIL https://phabricator.kde.org/D19913 AFFECTED FILES CMakeLists.txt autotests/coronatest.cpp autotests/packagestructuretest.cpp src/declarativeimports/calendar/daysmodel.cpp src/declarativeimports/calendar/eventpluginsmanager.cpp src/declarativeimports/core/datamodel.cpp src/declarativeimports/core/datamodel.h src/declarativeimports/core/datasource.cpp src/declarativeimports/core/iconitem.cpp src/declarativeimports/core/tooltipdialog.cpp src/declarativeimports/plasmacomponents/qmenu.cpp src/declarativeimports/plasmaextracomponents/fallbackcomponent.cpp src/plasma/containment.cpp src/plasma/corona.cpp src/plasma/datacontainer.cpp src/plasma/dataengine.cpp src/plasma/dataengineconsumer.cpp src/plasma/package.cpp src/plasma/pluginloader.cpp src/plasma/private/applet_p.cpp src/plasma/private/containment_p.cpp src/plasma/private/dataenginemanager.cpp src/plasma/private/theme_p.cpp src/plasma/private/timetracker.cpp src/plasma/scripting/scriptengine.cpp src/plasma/service.cpp src/plasma/svg.cpp src/plasmaquick/appletquickitem.cpp src/plasmaquick/configview.cpp src/plasmaquick/dialog.cpp src/plasmaquick/dialogshadows.cpp src/scriptengines/qml/plasmoid/appletinterface.cpp src/scriptengines/qml/plasmoid/containmentinterface.cpp tests/kplugins/plugintest.cpp To: mlaurent, dfaure Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
apol added a comment.
etc.
INLINE COMMENTS
> datamodel.cpp:309
>
> -foreach (const QString , m_dataSource->data()->keys()) {
> +const auto lst = m_dataSource->data()->keys();
> +for (const QString : lst) {
same?
Also this should clearly be using iterators, no?
> tooltipdialog.cpp:59
> //HACK: search our own import
> -foreach (const QString ,
> m_qmlObject->engine()->importPathList()) {
> +const auto lst = m_qmlObject->engine()->importPathList();
> +for (const QString : lst) {
same?
If we have to open an incidence for every comment this gets exhausting.
> containment.cpp:299
> }
> -foreach (Applet *applet, Containment::applets()) {
> +const auto lstApplets = Containment::applets();
> +for (Applet *applet : lstApplets) {
just applets?
> corona.cpp:132
> c->Applet::d->immutability = Types::Mutable;
> -foreach (Applet *a, c->applets()) {
> +const auto lstApplet = c->applets();
> +for (Applet *a : lstApplet) {
applets?
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
https://phabricator.kde.org/D19913
To: mlaurent, dfaure
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
mlaurent updated this revision to Diff 54920. mlaurent added a comment. Fix comment reported by Apol REPOSITORY R242 Plasma Framework (Library) CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19913?vs=54407=54920 BRANCH compile_without_foreach (branched from master) REVISION DETAIL https://phabricator.kde.org/D19913 AFFECTED FILES CMakeLists.txt autotests/coronatest.cpp autotests/packagestructuretest.cpp src/declarativeimports/calendar/daysmodel.cpp src/declarativeimports/calendar/eventpluginsmanager.cpp src/declarativeimports/core/datamodel.cpp src/declarativeimports/core/datamodel.h src/declarativeimports/core/datasource.cpp src/declarativeimports/core/iconitem.cpp src/declarativeimports/core/tooltipdialog.cpp src/declarativeimports/plasmacomponents/qmenu.cpp src/declarativeimports/plasmaextracomponents/fallbackcomponent.cpp src/plasma/containment.cpp src/plasma/corona.cpp src/plasma/datacontainer.cpp src/plasma/dataengine.cpp src/plasma/dataengineconsumer.cpp src/plasma/package.cpp src/plasma/pluginloader.cpp src/plasma/private/applet_p.cpp src/plasma/private/containment_p.cpp src/plasma/private/dataenginemanager.cpp src/plasma/private/theme_p.cpp src/plasma/private/timetracker.cpp src/plasma/scripting/scriptengine.cpp src/plasma/service.cpp src/plasma/svg.cpp src/plasmaquick/appletquickitem.cpp src/plasmaquick/configview.cpp src/plasmaquick/dialog.cpp src/plasmaquick/dialogshadows.cpp src/scriptengines/qml/plasmoid/appletinterface.cpp src/scriptengines/qml/plasmoid/containmentinterface.cpp tests/kplugins/plugintest.cpp To: mlaurent, dfaure Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
apol added a comment.
It's bad enough that we need to give things names because Qt keeps detaching
on foreach, I'd be giving at least more semantic names to these temporary
variables.
INLINE COMMENTS
> coronatest.cpp:149
>
> -foreach (auto cont, m_corona->containments()) {
> +const auto lst = m_corona->containments();
> +for (auto cont : lst) {
maybe call it containments? naming variables with a typename is bad (we even
are using auto to not mention it's a list!)
> datamodel.cpp:401
> if (list.first().canConvert()) {
> -foreach (const QVariant , list) {
> +for (const QVariant : qAsConst(list)) {
> const QVariantMap = item.value();
It already is const.
> pluginloader.cpp:567
> //info.service() to be valid and would crash otherwise
> -foreach (auto& md, plugins) {
> +for (auto& md : plugins) {
> auto pi = md.metaDataFileName().endsWith(QLatin1String(".json")) ?
> KPluginInfo(md) :
> KPluginInfo(KService::serviceByStorageId(md.metaDataFileName()));
shouldn't it be const?
REPOSITORY
R242 Plasma Framework (Library)
REVISION DETAIL
https://phabricator.kde.org/D19913
To: mlaurent, dfaure
Cc: apol, kde-frameworks-devel, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
mlaurent added a comment. Ping ?:) REPOSITORY R242 Plasma Framework (Library) REVISION DETAIL https://phabricator.kde.org/D19913 To: mlaurent, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D19913: [plasma-desktop] make it compiles without foreach
mlaurent created this revision. mlaurent added a reviewer: dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. mlaurent requested review of this revision. REVISION SUMMARY compile without foreach TEST PLAN autotest ok REPOSITORY R242 Plasma Framework (Library) BRANCH compile_without_foreach (branched from master) REVISION DETAIL https://phabricator.kde.org/D19913 AFFECTED FILES CMakeLists.txt autotests/coronatest.cpp autotests/packagestructuretest.cpp src/declarativeimports/calendar/daysmodel.cpp src/declarativeimports/calendar/eventpluginsmanager.cpp src/declarativeimports/core/datamodel.cpp src/declarativeimports/core/datamodel.h src/declarativeimports/core/datasource.cpp src/declarativeimports/core/iconitem.cpp src/declarativeimports/core/tooltipdialog.cpp src/declarativeimports/plasmacomponents/qmenu.cpp src/declarativeimports/plasmaextracomponents/fallbackcomponent.cpp src/plasma/containment.cpp src/plasma/corona.cpp src/plasma/datacontainer.cpp src/plasma/dataengine.cpp src/plasma/dataengineconsumer.cpp src/plasma/package.cpp src/plasma/pluginloader.cpp src/plasma/private/applet_p.cpp src/plasma/private/containment_p.cpp src/plasma/private/dataenginemanager.cpp src/plasma/private/theme_p.cpp src/plasma/private/timetracker.cpp src/plasma/scripting/scriptengine.cpp src/plasma/service.cpp src/plasma/svg.cpp src/plasmaquick/appletquickitem.cpp src/plasmaquick/configview.cpp src/plasmaquick/dialog.cpp src/plasmaquick/dialogshadows.cpp src/scriptengines/qml/plasmoid/appletinterface.cpp src/scriptengines/qml/plasmoid/containmentinterface.cpp tests/kplugins/plugintest.cpp To: mlaurent, dfaure Cc: kde-frameworks-devel, michaelh, ngraham, bruns
