dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.
I wonder if others agree that "a ? b() : c()" is bad form for void b() and
void c(), compared to an if(), or if it's just me.
INLINE COMMENTS
> kuitmarkup.cpp:1582
> + // unknown Unicode point, leave it as is
> + ok ? plain.append(c) : plain.append(match.captured(0));
> } else if (s->xmlEntities.contains(ent)) { // known entity
I don't really like ternary operators that don't return a value, this is really
an if().
(I guess the QChar/QString type difference is the reason why append(ok?c:...)
wouldn't work).
> ahmadsamir wrote in kuitmarkup.cpp:1572
> 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).
My statement doesn't apply to all classes obviously, the ctor/dtor could be
very expensive and then my statement is incorrect.
But QString is really just a wrapper around a QStringPrivate "d", and both
assignment or constructor (from another QString) just do some refcounting and
grab the "d" of the other QString, so it's the same.
> ahmadsamir wrote in kuitmarkup.cpp:1597
> 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. :)
I thought it was the resolved version, without entities (if they were
successfully resolved).
REPOSITORY
R249 KI18n
BRANCH
l-finalizeVisualText (branched from master)
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
