<table><tr><td style="">ahmadsamir 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/D26123">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/D26123#inline-147155">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kemailaddress.cpp:631</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Message from Giuseppe D'Angelo (author of QRegularExpression) : if you can depend from Qt 5.12, there's QRegularExpression::anchoredPattern instead of manual wrapping in \A and \z</p>
<p style="padding: 0; margin: 8px;">We can't require 5.12 yet but this could be a TODO to simplify this code slightly at some point.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Yep, I added the Qt 5.12 TODO in some QRegExp porting diffs but not all (kind of got boring (lots of QRegExp::exactMatch()), but that's not an excuse to be lazy and make it harder for the next guy who modifies that code.... :)).</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/D26123#inline-147156">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kemailaddress.cpp:639</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">{,3}</tt> would be <tt style="background: #ebebeb; font-size: 13px;">{0,3}</tt>, not <tt style="background: #ebebeb; font-size: 13px;">{1,3}</tt></p>
<p style="padding: 0; margin: 8px;">Would this allow to minimize the amount of changes made here? I'm worried about regressions.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">From the unit test, this is to match literal domains, [123.123.123.123]; but the old code would match:<br />
[.123.123.123]<br />
[..123.123]<br />
which seems wrong to me; as proven by this snippet in qtcreator (best way to test regex's is to just literally test them, that is more idiot-proof for me :)):</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);">const QString str = QStringLiteral("[1.1.1.1]");
const QString str2 = QStringLiteral("[.1.1.1]");
const QString str3 = QStringLiteral("[..1.1]");
const QString pattern = QStringLiteral("\\[[0-9]{,3}(\\.[0-9]{,3}){3}\\]");
QRegExp re(pattern);
qDebug() << re.exactMatch(str); // true
qDebug() << re.exactMatch(str2); // true
qDebug() << re.exactMatch(str3); // true</pre></div>
<p style="padding: 0; margin: 8px;">So I dare say even in the old code it should have been {1,3}.</p>
<p style="padding: 0; margin: 8px;">And I found that:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">{,3} in QRegExp is equivalent to {0,3}</li>
<li class="remarkup-list-item">{,3} in QRegularExpression is equivalent to {1,3}</li>
</ul>
<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);">const QString str = QStringLiteral("a");
const QString oldPattern = QStringLiteral("\\d{,3}"); // QRegExp _is_ old
QRegExp oldRe(oldPattern);
qDebug() << (oldRe.indexIn(str) != -1); // true, because it's {0,3}
const QString pattern = QStringLiteral("\\d{,3}");
QRegularExpression re(pattern);
qDebug() << re.match(str).hasMatch(); // false, because it's {1,3}</pre></div>
<p style="padding: 0; margin: 8px;">(^ candidate for QRegExp (its docs literally mention {,n}) to QRegularExpression porting notes in QRegularExpression docs, if you can pass that to Giuseppe D'Angelo :)).</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/D26123#inline-147156">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kemailaddress.cpp:639</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">{,3}</tt> would be <tt style="background: #ebebeb; font-size: 13px;">{0,3}</tt>, not <tt style="background: #ebebeb; font-size: 13px;">{1,3}</tt></p>
<p style="padding: 0; margin: 8px;">Would this allow to minimize the amount of changes made here? I'm worried about regressions.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I added two more rows to the literal domain test code and found that, my code is wrong too:</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);">addrRx += QStringLiteral("\\[([0-9]{1,3}\\.){1,3}([0-9]{1,3})\\]");
is wrong, it should be:
addrRx += QStringLiteral("\\[([0-9]{1,3}\\.){3}([0-9]{1,3})\\]");
^^^</pre></div>
<p style="padding: 0; margin: 8px;">I'll upload a new diff.</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/D26123#inline-147175">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kemailaddress.cpp:639</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I'm confused, you still have <tt style="background: #ebebeb; font-size: 13px;">1,3</tt> here, rather than <tt style="background: #ebebeb; font-size: 13px;">0,3</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I had replied to your comment but didn't "submit" my comments (stooopid, sorry).</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/D26123#inline-147157">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">kemailaddress.cpp:1119</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Also from Giuseppe : the <tt style="background: #ebebeb; font-size: 13px;">\\x0080-\\xFFFF</tt> requires changes to <tt style="background: #ebebeb; font-size: 13px;">\\x{0080}-\\x{FFFF}</tt></p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Thank him for me, and thank you for asking him to take a look at it :D; from <a href="https://perldoc.perl.org/perlre.html" class="remarkup-link" target="_blank" rel="noreferrer">https://perldoc.perl.org/perlre.html</a>:</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;">Similarly, \xnn, where nn are hexadecimal digits, matches the character whose native ordinal is nn. Again, not using exactly two digits is a recipe for disaster, but you can use \x{...} to specify any number of hex digits.</p></blockquote>
<p style="padding: 0; margin: 8px;">so, disaster averted.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R270 KCodecs</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D26123">https://phabricator.kde.org/D26123</a></div></div><br /><div><strong>To: </strong>ahmadsamir, Frameworks, dfaure, mlaurent, vkrause<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>