dhaumann added a comment.
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`.
INLINE COMMENTS
> fonthelpers.cpp:97
> // Generic fonts, in the inverse of desired order.
> - QStringList genericNames;
> - genericNames.append(QStringLiteral("Monospace"));
> - genericNames.append(QStringLiteral("Serif"));
> - genericNames.append(QStringLiteral("Sans Serif"));
> + const QStringList genericNames {
> + QStringLiteral("Monospace"),
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.
> kfontsizeaction.cpp:93
> + const auto actions = this->actions();
> + for (QAction* action : actions) {
> if (action->text() == test) {
Imho here `qAsConst(actions())` would be nicer to avoid this->.
> kviewstateserializer.h:134
> QStringList itemStrings;
> - Q_FOREACH(qint64 item, items)
> + fir (qint64 item : items)
> itemStrings << QString::number(item);
I think `fir` does not compile, does it?
REPOSITORY
R236 KWidgetsAddons
REVISION DETAIL
https://phabricator.kde.org/D23597
To: kossebau, #frameworks, cfeck
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
