dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kuitmarkup.cpp:1332
>
> -#define ENTITY_SUBRX "[a-z]+|#[0-9]+|#x[0-9a-fA-F]+"
> +const QString ENTITY_SUBRX = QStringLiteral("[a-z]+|#[0-9]+|#x[0-9a-fA-F]+");
>
This creates a QString at program startup. Please change it to
static const char s_entitySubRx[] = "...";
> kuitmarkup.cpp:1342
> // but do not touch & which forms an XML entity as it is.
> + // Regex is: ^([a-z]+|#[0-9]+|#x[0-9a-fA-F]+);
> + static const QRegularExpression restRx(QLatin1String("^(") +
> ENTITY_SUBRX + QLatin1String(");"));
Duplication :-)
Dangerous, if the actual char* is modified one day.
> kuitmarkup.cpp:1567
> if (format != Kuit::RichText) {
> - static QRegExp staticEntRx(QLatin1String("&(" ENTITY_SUBRX ");"));
> - QRegExp entRx = staticEntRx; // QRegExp not thread-safe
> - int p = entRx.indexIn(text);
> - QString plain;
> - while (p >= 0) {
> - QString ent = entRx.capturedTexts().at(1);
> - plain.append(text.midRef(0, p));
> - text.remove(0, p + ent.length() + 2);
> + // regex is: (&([a-z]+|#[0-9]+|#x[0-9a-fA-F]+);)
> + static const QRegularExpression entRx(QLatin1String("(&(") +
> ENTITY_SUBRX + QLatin1String(");)"));
(same)
> kuitmarkup.cpp:1572
> + QString text;
> + QString ent;
> + while (match.hasMatch()) {
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?
This is in no way more efficient, construction isn't very different from
assignment.
> kuitmarkup.cpp:1597
> if (format == Kuit::RichText) {
> - text = QStringLiteral("<html>") + text + QStringLiteral("</html>");
> + original = QLatin1String("<html>") + original +
> QLatin1String("</html>");
> }
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.
REPOSITORY
R249 KI18n
REVISION DETAIL
https://phabricator.kde.org/D26285
To: ahmadsamir, #frameworks, ilic, dfaure, mlaurent, aacid
Cc: kde-frameworks-devel, ltoscano, LeGast00n, GB_2, michaelh, ngraham, bruns
