<table><tr><td style="">dfaure accepted this revision.<br />dfaure added a comment.<br />This revision is now accepted and ready to land.
</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/D26285">View Revision</a></tr></table><br /><div><div><p>I wonder if others agree that "a ? b() : c()" is bad form for void b() and void c(), compared to an if(), or if it's just me.</p></div></div><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/D26285#inline-148349">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1582</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"> </span><span class="n"><span class="bright">c</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">QChar</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">ent</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">midRef</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #601200"><span class="bright">1</span></span><span class="bright"></span><span class="p"><span class="bright">).</span></span><span class="bright"></span><span class="n"><span class="bright">toInt</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span class="n"><span class="bright">ok</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">10</span></span><span class="bright"></span><span class="p"><span class="bright">));</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="p"><span class="bright">}</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">ok</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">plain</span><span class="p">.</span><span class="n">append</span><span class="p">(</span><span class="n">c</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="p">}</span> <span style="color: #aa4000">else</span> <span class="p">{</span> <span style="color: #74777d">// unknown Unicode point, leave as is</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">plain</span><span class="p">.</span><span class="n">append</span><span class="p">(</span><span class="n">QL1C</span><span class="p">(</span><span style="color: #766510">'&'</span><span class="p">)</span> <span style="color: #aa2211">+</span> <span class="n">ent</span> <span style="color: #aa2211">+</span> <span class="n">QL1C</span><span class="p">(</span><span style="color: #766510">';'</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// unknown Unicode point, leave it as is</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">ok</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">?</span></span><span class="bright"> </span><span class="n"><span class="bright">plain</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">append</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">c</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">:</span></span><span class="bright"> </span><span class="n"><span class="bright">plain</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">append</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">match</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">captured</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #601200"><span class="bright">0</span></span><span class="bright"></span><span class="p"><span class="bright">));</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span> <span style="color: #aa4000">else</span> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">s</span><span style="color: #aa2211">-></span><span class="n">xmlEntities</span><span class="p">.</span><span class="n">contains</span><span class="p">(</span><span class="n">ent</span><span class="p">))</span> <span class="p">{</span> <span style="color: #74777d">// known entity</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't really like ternary operators that don't return a value, this is really an if().</p>
<p style="padding: 0; margin: 8px;">(I guess the QChar/QString type difference is the reason why append(ok?c:...) wouldn't work).</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/D26285#inline-148317">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahmadsamir</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1572</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I wouldn't know about C, having never written any code with it.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">This is in no way more efficient, construction isn't very different from assignment.</p></blockquote>
<p style="padding: 0; margin: 8px;">That is the key I was missing (which means I should do some research, before "assuming" stuff).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">My statement doesn't apply to all classes obviously, the ctor/dtor could be very expensive and then my statement is incorrect.</p>
<p style="padding: 0; margin: 8px;">But QString is really just a wrapper around a QStringPrivate "d", and both assignment or constructor (from another QString) just do some refcounting and grab the "d" of the other QString, so it's the same.</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/D26285#inline-148318">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ahmadsamir</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1597</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I concede that changing var names makes reading the diff harder, and I should, if needed, have done it in a separate patch.</p>
<p style="padding: 0; margin: 8px;">My, weird, logic is that "original" is the original string that is being modified, v.s. text which is a temporary place to store the parts of original after processing them to prevent the match looping on one singular place in the string. Simply original didn't translate to constant in my head. (And "plain" didn't make sense, it's not plain when it may have "&blah4;" appended to it).</p>
<p style="padding: 0; margin: 8px;">Anyway, I'd rather change them back than argue. :)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I thought it was the resolved version, without entities (if they were successfully resolved).</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R249 KI18n</div></div></div><br /><div><strong>BRANCH</strong><div><div>l-finalizeVisualText (branched from master)</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D26285">https://phabricator.kde.org/D26285</a></div></div><br /><div><strong>To: </strong>ahmadsamir, Frameworks, ilic, dfaure, mlaurent, aacid<br /><strong>Cc: </strong>kde-frameworks-devel, ltoscano, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>