D26285: [KuitFormatterPrivate] Start porting QRegExp to QRegularExpression

[Ahmad Samir]

ahmadsamir added inline comments.

INLINE COMMENTS

> dfaure wrote in kuitmarkup.cpp:1342
> Duplication :-)
> 
> Dangerous, if the actual char* is modified one day.

Personally, I needed to see the actual regex spelled out like that to understand the rest of the code in this function, this:

  QLatin1String("^(") + QLatin1Strig(s_entitySubRx) + QLatin1String(");")

doesn't work as well for me, and especially so with finalizeVisualText(), where ENTITY_SUBRX is many lines away, and there are two capture groups used.

I thought of doing away with ENTITY_SUBRX and just construct the regex's in toVisualText() and  finalizeVisualText(), but the two must use the same pattern, except for the beginning, ^ and & respectively.

I can try lessening the danger by tweaking the comment.

> dfaure wrote in kuitmarkup.cpp:1572
> 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.

I wouldn't know about C, having never written any code with it.

> This is in no way more efficient, construction isn't very different from assignment.

That is the key I was missing (which means I should do some research, before "assuming" stuff).

> dfaure wrote in kuitmarkup.cpp:1597
> 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.

I concede that changing var names makes reading the diff harder, and I should, if needed, have done it in a separate patch.

My, weird, logic is that "original" is the original string that is being modified, v.s. text which is a temporary place to store the parts of original after processing them to prevent the match looping on one singular place in the string. Simply original didn't translate to constant in my head. (And "plain" didn't make sense, it's not plain when it may have "&blah4;" appended to it).

Anyway, I'd rather change them back than argue. :)

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/6e2b69ff/attachment.html>