dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.
I think this patch looks good (given we sat in front of this issue together,
I think I also know what's going on). There are still open questions, but
that's rather unrelated to this patch.
Tested with this patch in Kate, it seems to work. All kxmlgui unit tests
still pass.
I'm fine with committing. And btw, thanks a lot!
INLINE COMMENTS
> kxmlgui_unittest.cpp:305
> for (int i = 0; i < expectedActions.count(); ++i) {
> - QAction *action = actions[i];
> + if (i >= actions.count())
> + break;
Interesting, this looks as if there were invalid reads before?
> kxmlgui_unittest.cpp:411
> + for (int i = 0 ; i < 5 ; ++i) {
> + qDebug() << "addClient, iteration" << i;
> + factory.addClient(&partClient);
Is this qDebug() intentional?
> kxmlguifactory_p.cpp:717
> + // re-calculate the running default and client merging indices
> + // (especially important in case the QList data got reallocated due to
> growing!)
> m_state.currentDefaultMergingIt =
> parentNode->findIndex(defaultMergingName);
I'm not sure I get this in detail: but as you already mentioned yesterday, it
seems this saves iterators. It seems to me this code was written at a time (?)
when QList iterators were stable, but since a QList behaves similar to a
QVector for ints (iirc), this is rather dangerous, right? I guess that is what
you imply with your comment here. In that case, it's a wonder all this is still
working ;)
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D1924
EMAIL PREFERENCES
https://phabricator.kde.org/settings/panel/emailpreferences/
To: dfaure, svuorela, dhaumann
Cc: kde-frameworks-devel
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
