ahmadsamir added inline comments.
INLINE COMMENTS
> dfaure wrote in kemailaddress.cpp:631
> 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
>
> We can't require 5.12 yet but this could be a TODO to simplify this code
> slightly at some point.
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.... :)).
> dfaure wrote in kemailaddress.cpp:639
> `{,3}` would be `{0,3}`, not `{1,3}`
>
> Would this allow to minimize the amount of changes made here? I'm worried
> about regressions.
From the unit test, this is to match literal domains, [123.123.123.123]; but
the old code would match:
[.123.123.123]
[..123.123]
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
:)):
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
So I dare say even in the old code it should have been {1,3}.
And I found that:
- {,3} in QRegExp is equivalent to {0,3}
- {,3} in QRegularExpression is equivalent to {1,3}
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}
(^ candidate for QRegExp (its docs literally mention {,n}) to
QRegularExpression porting notes in QRegularExpression docs, if you can pass
that to Giuseppe D'Angelo :)).
> dfaure wrote in kemailaddress.cpp:639
> `{,3}` would be `{0,3}`, not `{1,3}`
>
> Would this allow to minimize the amount of changes made here? I'm worried
> about regressions.
I added two more rows to the literal domain test code and found that, my code
is wrong too:
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})\\]");
^^^
I'll upload a new diff.
> dfaure wrote in kemailaddress.cpp:639
> I'm confused, you still have `1,3` here, rather than `0,3`
I had replied to your comment but didn't "submit" my comments (stooopid, sorry).
> dfaure wrote in kemailaddress.cpp:1119
> Also from Giuseppe : the `\\x0080-\\xFFFF` requires changes to
> `\\x{0080}-\\x{FFFF}`
Thank him for me, and thank you for asking him to take a look at it :D; from
https://perldoc.perl.org/perlre.html:
> 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.
so, disaster averted.
REPOSITORY
R270 KCodecs
REVISION DETAIL
https://phabricator.kde.org/D26123
To: ahmadsamir, #frameworks, dfaure, mlaurent, vkrause
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
