Port toVisualText() and finalizeVisualText() to use QRegularExpression.
Replace ENTITY_SUBRX macro with a static const char*.
Details
Details
- Reviewers
ilic dfaure mlaurent aacid - Group Reviewers
Frameworks - Maniphest Tasks
- T12279: Port frameworks away from QRegExp
- Commits
- R249:f9b932d254c4: [KuitFormatterPrivate] Start porting QRegExp to QRegularExpression
make && ctest
Diff Detail
Diff Detail
- Repository
- R249 KI18n
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| src/kuitmarkup.cpp | ||
|---|---|---|
| 1332 | This creates a QString at program startup. Please change it to static const char s_entitySubRx[] = "..."; | |
| 1342 | Duplication :-) Dangerous, if the actual char* is modified one day. | |
| 1572–1575 | (same) | |
| 1577 | 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? | |
| 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. | |
| src/kuitmarkup.cpp | ||
|---|---|---|
| 1342 | 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. | |
| 1577 | I wouldn't know about C, having never written any code with it.
That is the key I was missing (which means I should do some research, before "assuming" stuff). | |
| 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. :) | |
Comment Actions
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.
| src/kuitmarkup.cpp | ||
|---|---|---|
| 1577 | 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. | |
| 1588 | 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). | |
| 1597 | I thought it was the resolved version, without entities (if they were successfully resolved). | |
Comment Actions
Ternary that doesn't return a value is weird, better avoid.
"Grammar fixes" in comments add noise to the diff, be kind to reviewers.
| src/kuitmarkup.cpp | ||
|---|---|---|
| 1597 | If the entity is unknown it'll be left as is, I don't know how often that actually happens though. | |