D26285: [KuitFormatterPrivate] Start porting QRegExp to QRegularExpression

Wed Jan 1 10:04:37 GMT 2020

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200101/0eba07d6/attachment-0001.html>