<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure added inline comments.<br />This revision now requires changes to proceed.
</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><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-148270">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1332</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 style="color: #304a96"><span class="bright">#define</span> ENTITY_SUBRX "[a-z]+|#[0-9]+|#x[0-9a-fA-F]+"</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n"><span class="bright">QString</span></span> <span class="n">ENTITY_SUBRX</span> <span class="bright"></span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">QStringLiteral</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span style="color: #766510">"[a-z]+|#[0-9]+|#x[0-9a-fA-F]+"<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">);</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This creates a QString at program startup. Please change it to</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">static const char s_entitySubRx[] = "...";</pre></div></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-148271">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1342</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; "> <span style="color: #74777d">// but do not touch & which forms an XML entity as it is.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// Regex is: ^([a-z]+|#[0-9]+|#x[0-9a-fA-F]+);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">static</span> <span style="color: #aa4000">const</span> <span class="n">QRegularExpression</span> <span style="color: #004012">restRx</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"^("</span><span class="p">)</span> <span style="color: #aa2211">+</span> <span class="n">ENTITY_SUBRX</span> <span style="color: #aa2211">+</span> <span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">")</span><span class="p">;</span><span style="color: #766510">"));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Duplication :-)</p>
<p style="padding: 0; margin: 8px;">Dangerous, if the actual char* is modified one day.</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-148272">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1567</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; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">format</span> <span style="color: #aa2211">!=</span> <span class="n">Kuit</span><span style="color: #aa2211">::</span><span class="n">RichText</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="bright"></span><span style="color: #aa4000"><span class="bright">static</span></span><span class="bright"> </span><span class="n"><span class="bright">QRegExp</span></span><span class="bright"> </span><span class="n"><span class="bright">staticEntRx</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QLatin1String</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"&("</span></span><span class="bright"> </span><span class="n"><span class="bright">ENTITY_SUBRX</span></span><span class="bright"> </span><span style="color: #766510"><span class="bright">");"</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="n">QReg<span class="bright">Exp</span></span> <span class="n">entRx<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">staticEntRx</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span><span class="bright"> </span><span style="color: #74777d"><span class="bright">// QRegExp not thread-safe</span></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">// regex is: (&([a-z]+|#[0-9]+|#x[0-9a-fA-F]+);)</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">static</span></span><span class="bright"> </span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n">QReg<span class="bright">ularExpression</span></span> <span class="n">entRx<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QLatin1String</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">"(&("</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">ENTITY_SUBRX</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">QLatin1String</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #766510"><span class="bright">");)"</span></span><span class="bright"></span><span class="p"><span class="bright">));</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">(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-148274">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1572</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 style="color: #aa4000"><span class="bright">while</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">p</span></span><span class="bright"> </span><span style="color: #aa2211"><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><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="n">QString</span> <span class="n">ent<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">entRx</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">capturedTexts</span></span><span class="bright"></span><span class="p"><span class="bright">().</span></span><span class="bright"></span><span class="n"><span class="bright">at</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>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <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">text</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">0</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">p</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(151, 234, 151, .6);"> <span class="bright"></span><span class="n"><span class="bright">QString</span></span><span class="bright"> </span><span class="n"><span class="bright">text</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(151, 234, 151, .6);"> <span class="n">QString</span> <span class="n">ent</span><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: #aa4000"><span class="bright">while</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">hasMatch</span></span><span class="bright"></span><span class="p"><span class="bright">())</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Please try to always declare the variables where they are used. The old code said "QString ent = ...", why does the new code look more like C, declaring variables outside the loop?<br />
This is in no way more efficient, construction isn't very different from assignment.</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-148273">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kuitmarkup.cpp:1597</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; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">format</span> <span style="color: #aa2211">==</span> <span class="n">Kuit</span><span style="color: #aa2211">::</span><span class="n">RichText</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="bright"></span><span class="n"><span class="bright">text</span></span> <span style="color: #aa2211">=</span> <span class="n">QString<span class="bright">Literal</span></span><span class="p">(</span><span style="color: #766510">"<html>"</span><span class="p">)</span> <span style="color: #aa2211">+</span> <span class="bright"></span><span class="n"><span class="bright">text</span></span> <span style="color: #aa2211">+</span> <span class="n">QString<span class="bright">Literal</span></span><span class="p">(</span><span style="color: #766510">"</html>"</span><span class="p">);</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">original</span></span> <span style="color: #aa2211">=</span> <span class="n">Q<span class="bright">Latin1</span>String</span><span class="p">(</span><span style="color: #766510">"<html>"</span><span class="p">)</span> <span style="color: #aa2211">+</span> <span class="bright"></span><span class="n"><span class="bright">original</span></span> <span style="color: #aa2211">+</span> <span class="n">Q<span class="bright">Latin1</span>String</span><span class="p">(</span><span style="color: #766510">"</html>"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">So it's not the "original" value anymore. Why the renaming of plain to text, and of text to original? It makes the diff harder to review (but if you convince me that it makes the code easier to read, that's fine). I'm just not sure that "original" for something that is modified in many places (line 1592 and here) makes sense as a name.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R249 KI18n</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>