Bulk port away from foreach
ClosedPublic
Actions

Authored by kossebau on Aug 31 2019, 5:56 AM.

Details

Reviewers

cfeck
dhaumann

Group Reviewers

Frameworks

Commits

R236:549fc06ffa21: Bulk port away from foreach

Summary

Few are left as they are with code which needs further analysis
and posssibly fixes

Test Plan

Tests still pass, all widgets quickly tested in occurences.

Diff Detail

Repository

R236 KWidgetsAddons

Lint

Automatic diff as part of commit; lint not applicable.

Unit

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

kossebau created this revision.Aug 31 2019, 5:56 AM

Restricted Application added a project: Frameworks. · View Herald TranscriptAug 31 2019, 5:56 AM

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

kossebau requested review of this revision.Aug 31 2019, 5:56 AM

Harbormaster completed remote builds in B15883: Diff 65032.Aug 31 2019, 5:56 AM

I think the patch is fine: +1

Please address/comment on fir :) besides that, another review won't hurt, since you simplify the code in 1-2 places, i.e. the changes are slightly more than just the transition to for.

src/fonthelpers.cpp
97–101	Optionally, you could even write: static const auto genericNames = { QLatin1String("..."), ... }; This initializer list will then even not require any memory allocation. You don't have.contains() later, though... You'd need to simply use std::find() semantics.
src/kfontsizeaction.cpp
93	Imho here `qAsConst(actions())` would be nicer to avoid this->.
src/kviewstateserializer.h
134	I think `fir` does not compile, does it?

PS: could you do the same for kate.git ? :-D

Thanks for review @dhaumann :) Well, if you like I can give KTextEditor a try, motivated there with my KDevelop hat on :) Let's see when I am sleepless at the computer next time ;)

src/fonthelpers.cpp
97–101	Yes, it was the contains() which was my motivation here to stay with QStringList.
src/kfontsizeaction.cpp
93	Will test is this works, but IIRC actions() returned a normal left-side value thingie, which qAsConst does not want to take.
src/kviewstateserializer.h
134	That was... left as exercise for the person copying the code from the docs ;) Alright, not. Well spotted.

cfeck added inline comments.Sep 1 2019, 1:58 AM

autotests/kacceleratormanagertest.cpp
35	Please use KF5 coding style: `Type var` instead of `Type var` (also for `&` references),

align * & & with var name, not type, by current KF coding style
fix "fir" for "for"

Harbormaster completed remote builds in B15920: Diff 65089.Sep 1 2019, 6:36 AM

kossebau added inline comments.Sep 1 2019, 6:50 AM

src/kfontsizeaction.cpp
93	Yes, `qAsConst(actions())` does not work, as actions() is a r-value type, which qAsConst does not take (move overload of qAsConst explicitly deleted), following std::as_const here, to not run into dangling pointers.

I think this patch is good to go in.

This revision is now accepted and ready to land.Sep 1 2019, 8:08 AM

Thanks. In any case, would wait for after 5.62 tagging/branching, as by experience there still might be one or the other regression slipped in with porting from foreach, so having some weeks of testing from more people running git master makes me feel better, before this gets rolled to the masses :)

btw, already started to do the same to ktexteditor, so in some days you kate devs shall enjoy the review of a similar patch...

Closed by commit R236:549fc06ffa21: Bulk port away from foreach (authored by kossebau). · Explain WhySep 7 2019, 11:59 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			autotests/kacceleratormanagertest.cpp (3 lines)
M			autotests/kdatetimeedittest.cpp (6 lines)
M			src/common_helpers.cpp (2 lines)
M			src/fonthelpers.cpp (11 lines)
M			src/kacceleratormanager.cpp (17 lines)
M			src/kactionselector.cpp (8 lines)
M			src/kcharselect.cpp (42 lines)
M			src/kcharselectdata.cpp (8 lines)
M			src/kcollapsiblegroupbox.cpp (3 lines)
M			src/kcolumnresizer.cpp (6 lines)
M			src/kdatetimeedit.cpp (8 lines)
M			src/kfontaction.cpp (6 lines)
M			src/kfontrequester.cpp (4 lines)
M			src/kfontsizeaction.cpp (3 lines)
M			src/kmessagebox_p.cpp (3 lines)
M			src/kmessagewidget.cpp (6 lines)
M			src/kmimetypechooser.cpp (7 lines)
M			src/kselectaction.cpp (46 lines)
M			src/kselectaction_p.h (4 lines)
M			src/ksqueezedtextlabel.cpp (4 lines)
M			src/ktimecombobox.cpp (4 lines)
M			src/ktoolbarlabelaction.cpp (12 lines)
M			src/kviewstateserializer.h (4 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	65032	8b6b95d		Aug 31 2019, 5:56 AM	★	★
Diff 2	65089	8b6b95d	* align * & & with var name, not type, by current KF coding style	Sep 1 2019, 6:36 AM	★	★
Diff 3	65614	ba57646	R236:549fc06ffa21a2f69c88afcb958c9428a2fd7633	Sep 7 2019, 11:59 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
50c88b7cde59	9522343e978f	8b6b95d8df1c	Friedrich W. H. Kossebau	Bulk port away from foreach (Show More…)	Aug 31 2019, 5:55 AM

Bulk port away from foreachClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 65614

autotests/kacceleratormanagertest.cpp

autotests/kdatetimeedittest.cpp

src/common_helpers.cpp

src/fonthelpers.cpp

src/kacceleratormanager.cpp

src/kactionselector.cpp

src/kcharselect.cpp

src/kcharselectdata.cpp

src/kcollapsiblegroupbox.cpp

src/kcolumnresizer.cpp

src/kdatetimeedit.cpp

src/kfontaction.cpp

src/kfontrequester.cpp

src/kfontsizeaction.cpp

src/kmessagebox_p.cpp

src/kmessagewidget.cpp

src/kmimetypechooser.cpp

src/kselectaction.cpp

src/kselectaction_p.h

src/ksqueezedtextlabel.cpp

src/ktimecombobox.cpp

src/ktoolbarlabelaction.cpp

src/kviewstateserializer.h

Bulk port away from foreach
ClosedPublic
Actions

Revision Contents
Changeset List