<table><tr><td style="">dfaure marked 2 inline comments as done.<br />dfaure added inline comments.</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D1924" rel="noreferrer">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1924#inline-7371" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kxmlgui_unittest.cpp:305</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Interesting, this looks as if there were invalid reads before?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Not exactly. QList's [i] asserts when called out of bounds. It didn't happen before, because when the test passes, actions == expectedActions ;)<br />
It simply happened to me while working on the unittest and getting less actions than expected.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1924#inline-7378" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">svuorela</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kxmlgui_unittest.cpp:306</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Isn't it better to move the last QCOMPARE(count,count); up first?  Or is it to  be able to easier debug if something went wrong?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Right it's at the end because when "A,B,C" is matched against "A,D,B,C" I'd rather be told "at position 1 I expected D but got B" than "expected 3, got 4". Easier to debug.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1924#inline-7372" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kxmlgui_unittest.cpp:411</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Is this qDebug() intentional?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">It was, but ok, let's remove it.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1924#inline-7385" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">svuorela</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kxmlguibuilder.h:83</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">And use what instead? maybe mark deprecated?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">And use nothing, it was only used internally and it's not used anymore.</p>

<p style="padding: 0; margin: 8px;">AFAICS nobody uses this, but yes, good idea to add deprecation macro to make sure it stays that way.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D1924#inline-7374" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dhaumann</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kxmlguifactory_p.cpp:717</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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 ;)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I actually looked at the kdelibs3 code and it was using QValueList already ;)</p>

<p style="padding: 0; margin: 8px;">Yes it's dangerous to store iterators into a QList or QVector, however what this code does (right under this comment) is to recalculate these two iterators when adding to the QList, which solves exactly that risk. My commit simply makes that more explicit in the comment.</p>

<p style="padding: 0; margin: 8px;">Hmm, I forgot to check for code _removing_ from the list. Although I doubt QList reallocates when shrinking. Checked now, there isn't.</p>

<p style="padding: 0; margin: 8px;">BTW I also searched bugzilla for crashes related to ContainerNode and BuildHelper and didn't find anything ;)</p></div></div></div></div></div><br /><div><strong>BRANCH</strong><div><div>master</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D1924" rel="noreferrer">https://phabricator.kde.org/D1924</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>dfaure, svuorela, dhaumann<br /><strong>Cc: </strong>kde-frameworks-devel<br /></div>