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
