Differential D20489

[KIO] Make it compile without foreach (Step 1)
ClosedPublic
Actions

Authored by mlaurent on Apr 12 2019, 11:47 AM.

Details

Reviewers

dfaure

Commits

R241:3f482d8981b0: [KIO] Make it compile without foreach (Step 1)

Summary

Port a lot of foreach/FOREACH (still 220 items)
It will be deprecated in qt6.

NO_CHANGELOG

Test Plan

autotest ok

Diff Detail

Repository

R241 KIO

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

mlaurent created this revision.Apr 12 2019, 11:47 AM

Restricted Application added a project: Frameworks. · View Herald TranscriptApr 12 2019, 11:47 AM

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

mlaurent requested review of this revision.Apr 12 2019, 11:47 AM

Harbormaster completed remote builds in B10766: Diff 56058.Apr 12 2019, 11:47 AM

Please fix the title

what is the problem with title ?

In D20489#448703, @mlaurent wrote:

what is the problem with title ?

"Make to compile without foreach"

it's missing words or some have to be changed.
The summary also lacks explanations *why* this change is needed (like almost *all* the commits in the pim repositories)

we're not using SVN anymore where a commit had to be pushed before preparing the next one, there's time to explain things.

mlaurent retitled this revision from [KIO] Make to compile without foreach (Step 1) to [KIO] Make it compile without foreach (Step 1).Apr 12 2019, 1:23 PM

mlaurent edited the summary of this revision. (Show Details)

cgiboudeaux: the Qt documentation says that foreach is deprecated, what do you suggest Laurent should add to the commit log? Just "it's deprecated", a copy/paste of the Qt docu, or https://www.kdab.com/goodbye-q_foreach/ ?

autotests/globaltest.cpp
110	qAsConst? I guess QFETCH doesn't make the variables const.
src/widgets/kdirmodel.cpp
263	qAsConst

This revision now requires changes to proceed.Apr 13 2019, 10:50 AM

In D20489#449133, @dfaure wrote:

cgiboudeaux: the Qt documentation says that foreach is deprecated, what do you suggest Laurent should add to the commit log? Just "it's deprecated", a copy/paste of the Qt docu, or https://www.kdab.com/goodbye-q_foreach/ ?

"It will be deprecated in Qt6" tells packagers why this commit is done.
In a perfect world, developers would write changelog files with changes worth mentioning when packagers prepare the updates.
As this is not close to happen, we extract git logs and spend an insane amount of time cleaning meaningless commit messages for *each* released tarball (and specially the pim ones) and we repeat that for *every* release.

mlaurent added inline comments.Apr 13 2019, 12:09 PM

autotests/globaltest.cpp
110	indeed.
src/widgets/kdirmodel.cpp
263	Ah yep I thought that it was the args from method. I will fix it soon

Yep, but extracting changelogs from commit logs can be mostly automated, including filtering out many commits with the same first line.
It's what I do in git@git.kde.org:sysadmin/release-tools branch frameworks/5.0 file parse_changelogs.pl.

But yeah, if people could add NO_CHANGELOG somewhere in the commit log it would make this easier. Laurent, can you do that for mechanical commits such as this one?

mlaurent edited the summary of this revision. (Show Details)Apr 13 2019, 1:04 PM

Add missing qAsConst

Harbormaster completed remote builds in B10803: Diff 56136.Apr 13 2019, 1:07 PM

"But yeah, if people could add NO_CHANGELOG somewhere in the commit log it would make this easier. Laurent, can you do that for mechanical commits such as this one?"

no problem for adding [NO_CHANGELOG] in commit message

FYI NO_CHANGELOG doesn't have to be in the first line (which would make noise in phab review titles for new requests, etc). It can be a line of its own, for example towards the end of the commit log.

Thanks!

This revision is now accepted and ready to land.Apr 13 2019, 1:47 PM

mlaurent edited the summary of this revision. (Show Details)Apr 13 2019, 4:27 PM

Closed by commit R241:3f482d8981b0: [KIO] Make it compile without foreach (Step 1) (authored by mlaurent). · Explain WhyApr 13 2019, 4:27 PM

This revision was automatically updated to reflect the committed changes.

In D20489#449216, @dfaure wrote:

FYI NO_CHANGELOG doesn't have to be in the first line (which would make noise in phab review titles for new requests, etc). It can be a line of its own, for example towards the end of the commit log.

Thanks!

Isn't that what GIT_SILENT is for? Do we really need another keyword?

GIT_SILENT is "trivial, don't look at the commit" -- at least it used to be. Not really applicable to commits such as this one....

hmmmm, ok, i don't know why we wouldn't want things like this to now show in the autogenerated changelog, but ok, your call :)

I view the KF5 changelog as the list of things that can be useful to the users of the frameworks (i.e. application developers).
When we add API, fix a bug, or change dependencies, that's useful for them to know.
When we repair a unittest, fix typos in comments, port away from deprecated methods and so on, I don't see how that is useful for the application developers to know. To me it would just be noise in the changelog, it doesn't affect them.
Sure, in the long run it means we're maintaining the stuff and making sure it will still work with future versions of Qt, but they'll get notified of that when the time comes anyway.

Do we understand all possible regressions that this can cause? If yes, where are they documented so that we can verify? See bug 406426.

Yes, the pitfalls are well known.

don't modify the container being iterated upon (from inside the loop)
don't use qAsConst on temporaries

and for performance reasons, "don't use range-for over a non-const container", but that's of course not as bad as 1) and 2) which lead to crashes.

https://www.kdab.com/goodbye-q_foreach/ has a lot of details on point 2) in the comments.

@mlaurent Hi, sorry to default to you here, but given the risks with foreach porting (I have done my own share of mistakes in such despite all concentration :) ) https://bugs.kde.org/show_bug.cgi?id=408801 might be triggered by one of this here, have not yet investigated more.
With your mind perhaps still with memories about the code from the foreach porting work, could you given that bug take a look where this might be due to containers being modified in a range-for loop?

I will look at it

Revision Contents
Changeset List

			Path	Packages
M			autotests/batchrenamejobtest.cpp (6 lines)
M			autotests/clipboardupdatertest.cpp (2 lines)
M			autotests/dataprotocoltest.cpp (2 lines)
M			autotests/fileundomanagertest.cpp (4 lines)
M			autotests/globaltest.cpp (2 lines)
M			autotests/http/httpheaderdispositiontest.cpp (2 lines)
M			autotests/jobtest.cpp (8 lines)
M			autotests/kdirmodeltest.cpp (2 lines)
M			autotests/kfilewidgettest.cpp (2 lines)
M			autotests/kmountpointtest.cpp (4 lines)
M			autotests/kurifiltersearchprovideractionstest.cpp (2 lines)
M			autotests/kurlcompletiontest.cpp (2 lines)
M			src/core/kcoredirlister.cpp (8 lines)
M			src/core/kfileitemlistproperties.cpp (4 lines)
M			src/core/kioglobal_p.cpp (4 lines)
M			src/core/kprotocolmanager.cpp (12 lines)
M			src/core/ksslcertificatemanager.cpp (12 lines)
M			src/core/ktcpsocket.cpp (6 lines)
M			src/core/slavebase.cpp (2 lines)
M			src/core/tcpslavebase.cpp (2 lines)
M			src/filewidgets/kdiroperator.cpp (8 lines)
M			src/filewidgets/kencodingfiledialog.cpp (2 lines)
M			src/filewidgets/kfilecopytomenu.cpp (4 lines)
M			src/filewidgets/kfilemetapreview.cpp (2 lines)
M			src/filewidgets/kfileplacesmodel.cpp (8 lines)
M			src/filewidgets/kfilepreviewgenerator.cpp (4 lines)
M			src/filewidgets/kfilewidget.cpp (6 lines)
M			src/filewidgets/knewfilemenu.cpp (6 lines)
M			src/filewidgets/kurlnavigatorbutton.cpp (2 lines)
M			src/filewidgets/kurlnavigatorplacesselector.cpp (2 lines)
M			src/filewidgets/kurlnavigatorprotocolcombo.cpp (2 lines)
M			src/ioslaves/file/file.cpp (2 lines)
M			src/ioslaves/file/file_unix.cpp (2 lines)
M			src/ioslaves/http/http_cache_cleaner.cpp (2 lines)
M			src/ioslaves/http/httpauthentication.cpp (4 lines)
M			src/ioslaves/http/kcookiejar/kcookiejar.cpp (12 lines)
M			src/ioslaves/http/kcookiejar/kcookieserver.cpp (6 lines)
M			src/kcms/kio/kcookiesmanagement.cpp (2 lines)
M			src/kcms/kio/kproxydlg.cpp (2 lines)
M			src/kiod/kiod_main.cpp (2 lines)
M			src/kpac/script.cpp (6 lines)
M			src/kpasswdserver/autotests/kpasswdservertest.cpp (2 lines)
M			src/kpasswdserver/kpasswdserver.cpp (2 lines)
M			src/kssld/kssld.cpp (2 lines)
M			src/protocoltojson/main.cpp (2 lines)
M			src/urifilters/ikws/ikwsopts.cpp (6 lines)
M			src/urifilters/ikws/searchprovider.cpp (2 lines)
M			src/urifilters/ikws/searchproviderdlg.cpp (4 lines)
M			src/widgets/accessmanager.cpp (4 lines)
M			src/widgets/accessmanagerreply_p.cpp (2 lines)
M			src/widgets/dropjob.cpp (6 lines)
M			src/widgets/jobuidelegate.cpp (4 lines)
M			src/widgets/kdirmodel.cpp (8 lines)
M			src/widgets/kfileitemactions.cpp (18 lines)
M			src/widgets/kpropertiesdialog.cpp (4 lines)
M			src/widgets/ksslinfodialog.cpp (8 lines)
M			src/widgets/kurifilter.cpp (2 lines)
M			src/widgets/kurlcompletion.cpp (8 lines)
M			src/widgets/paste.cpp (2 lines)
M			src/widgets/previewjob.cpp (6 lines)
M			tests/getalltest.cpp (12 lines)
M			tests/udsentrybenchmark.cpp (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	56058	ef67952		Apr 12 2019, 11:47 AM	★	★
Diff 2	56136	ef67952	Add missing qAsConst	Apr 13 2019, 1:07 PM	★	★
Diff 3	56150	f3b0f22	R241:3f482d8981b0d1d0ea3b3805639f98444e85944b	Apr 13 2019, 4:27 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
e1465b1f983f	f7e08fe668aa	ef67952391dc	Laurent Montel	[KIO] Make to compile without foreach (Step 1) (Show More…)	Apr 12 2019, 11:47 AM

[KIO] Make it compile without foreach (Step 1)ClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 56150

autotests/batchrenamejobtest.cpp

autotests/clipboardupdatertest.cpp

autotests/dataprotocoltest.cpp

autotests/fileundomanagertest.cpp

autotests/globaltest.cpp

autotests/http/httpheaderdispositiontest.cpp

autotests/jobtest.cpp

autotests/kdirmodeltest.cpp

autotests/kfilewidgettest.cpp

autotests/kmountpointtest.cpp

autotests/kurifiltersearchprovideractionstest.cpp

autotests/kurlcompletiontest.cpp

src/core/kcoredirlister.cpp

src/core/kfileitemlistproperties.cpp

src/core/kioglobal_p.cpp

src/core/kprotocolmanager.cpp

src/core/ksslcertificatemanager.cpp

src/core/ktcpsocket.cpp

src/core/slavebase.cpp

src/core/tcpslavebase.cpp

src/filewidgets/kdiroperator.cpp

src/filewidgets/kencodingfiledialog.cpp

src/filewidgets/kfilecopytomenu.cpp

src/filewidgets/kfilemetapreview.cpp

src/filewidgets/kfileplacesmodel.cpp

src/filewidgets/kfilepreviewgenerator.cpp

src/filewidgets/kfilewidget.cpp

src/filewidgets/knewfilemenu.cpp

src/filewidgets/kurlnavigatorbutton.cpp

src/filewidgets/kurlnavigatorplacesselector.cpp

src/filewidgets/kurlnavigatorprotocolcombo.cpp

src/ioslaves/file/file.cpp

src/ioslaves/file/file_unix.cpp

src/ioslaves/http/http_cache_cleaner.cpp

src/ioslaves/http/httpauthentication.cpp

src/ioslaves/http/kcookiejar/kcookiejar.cpp

src/ioslaves/http/kcookiejar/kcookieserver.cpp

src/kcms/kio/kcookiesmanagement.cpp

src/kcms/kio/kproxydlg.cpp

src/kiod/kiod_main.cpp

src/kpac/script.cpp

src/kpasswdserver/autotests/kpasswdservertest.cpp

src/kpasswdserver/kpasswdserver.cpp

src/kssld/kssld.cpp

src/protocoltojson/main.cpp

src/urifilters/ikws/ikwsopts.cpp

src/urifilters/ikws/searchprovider.cpp

src/urifilters/ikws/searchproviderdlg.cpp

src/widgets/accessmanager.cpp

src/widgets/accessmanagerreply_p.cpp

src/widgets/dropjob.cpp

src/widgets/jobuidelegate.cpp

src/widgets/kdirmodel.cpp

src/widgets/kfileitemactions.cpp

src/widgets/kpropertiesdialog.cpp

src/widgets/ksslinfodialog.cpp

src/widgets/kurifilter.cpp

src/widgets/kurlcompletion.cpp

src/widgets/paste.cpp

src/widgets/previewjob.cpp

tests/getalltest.cpp

tests/udsentrybenchmark.cpp

[KIO] Make it compile without foreach (Step 1)
ClosedPublic
Actions

Revision Contents
Changeset List