Use Int instead of QString in TableRow
ClosedPublic

Authored by wojnilowicz on Jun 10 2017, 7:38 AM.

Details

Summary

The main purpose of this patch is to reduce memory and cpu usage while rendering list table.
It has been done through:

  1. use of QLatin1String where applicable,
  2. moving i18nHeaders to function, because not all headers are needed every time table is rendered,
  3. moving column type detection to function, because of the same reason as above,
  4. using Int instead of QString as key in TableRow, because it's less error prone, faster, and smaller,
  5. applying multiarg instead of arg chaining, as it doesn't create temporary strings.

Diff Detail

Repository
R261 KMyMoney
Lint
Lint Skipped
Unit
Unit Tests Skipped
wojnilowicz created this revision.Jun 10 2017, 7:38 AM
wojnilowicz set the repository for this revision to R261 KMyMoney.

Your changes improve the code a lot! They make this code better in so many ways, thank you!

kmymoney/reports/listtable.cpp
111

Instead of QString(QLatin1String("…")) you can use QString::fromLatin1String("…") (Qt5 Doc).

112

Did you notice a difference between

csv += QLatin1String("\"Report: " + m_config.name() + "\"\n");

and

csv.append(QString(QLatin1String("\"Report: %1\"\n")).arg(m_config.name()));?

231

If braces are used once in a if, else if, else statement, they should be used always, even in not required (I know, old code).

625

You could just use return i18n("Date"). Then you save many breaks and maybe the optimizer will be even faster then.

kmymoney/reports/listtable.h
64

You used a prefix here (ct). Maybe this enum is a good candidate for scoped enumerations.

106–107

If you want to speed up, a std::vector could be helpful here. Even std::array is possible because the number of possible criteria is know at compile time and not too long.

wojnilowicz marked 6 inline comments as done.Jun 20 2017, 5:03 PM
wojnilowicz added inline comments.
kmymoney/reports/listtable.cpp
112

In first case you have two temporary strings, which build third (target) string.
In second case only one temporary string to build second (target) string.

When it comes to "+=" vs "append" they're probably the same except in second you can see (through code auto-completion) what arguments are accepted.

625

I hope it means RVO.

kmymoney/reports/listtable.h
64

Let's put this on TODO list because now it would be conflicting with my next patches, which come after this patch.

106–107

I've done it with QVector. I believe it's good enough.

wojnilowicz marked 4 inline comments as done.Jun 20 2017, 5:05 PM

Your changes improve the code a lot! They make this code better in so many ways, thank you!

Nice to hear that and be noticed. You and Thomas are very motivating in that field :)

Updated after remarks from Christian.

Is this patch ready to land now?

tbaumgart accepted this revision as: tbaumgart.Jun 20 2017, 6:26 PM
tbaumgart added a subscriber: tbaumgart.

Looks good to me. Very nice job. Now we need to add some new reports that group upon the cost center.

This revision is now accepted and ready to land.Jun 20 2017, 6:26 PM
This revision was automatically updated to reflect the committed changes.