Create d-pointer for MyMoneyObject
AbandonedPublic

Authored by wojnilowicz on Dec 2 2017, 1:01 PM.

Details

Test Plan

All tests pass.

Diff Detail

Repository
R261 KMyMoney
Lint
Lint Skipped
Unit
Unit Tests Skipped
wojnilowicz requested review of this revision.Dec 2 2017, 1:01 PM
wojnilowicz created this revision.
broulik added a subscriber: broulik.Dec 2 2017, 1:59 PM

Instead of comparing to QString() check isEmpty()

tbaumgart requested changes to this revision.Dec 2 2017, 2:01 PM
tbaumgart added a subscriber: tbaumgart.

Compiles here and tests run successfully.

kmymoney/kmymoney.cpp
7371

Isn't

if(job.id().isEmpty())

even better here?

7400

Same here as above (!empty()). Also, I noticed that the Q_ASSERT stuff does not do anything here in my environment. What would I need to do see if it fails?

kmymoney/mymoney/mymoneyobject_p.h
3

seems to be the wrong name here

kmymoney/mymoney/mymoneypayee.h
47–48

This looks a bit different in other classes. Why don't you need

Q_DECLARE_PRIVATE_D(MyMoneyObject::d_ptr, MyMoneyPayee)

here?

kmymoney/mymoney/mymoneytag.h
42–43

Same question as with MyMoneyPayee.

kmymoney/mymoney/storage/tests/mymoneydatabasemgr-test.cpp
673

Better use

s.d_func()->clearId();

here

kmymoney/mymoney/tests/onlinejob-test.cpp
35

Better use

QCOMPARE(job.id(), QString());

here.

This revision now requires changes to proceed.Dec 2 2017, 2:01 PM
wojnilowicz marked 6 inline comments as done.Dec 2 2017, 2:54 PM
wojnilowicz added inline comments.
kmymoney/kmymoney.cpp
7400

I don't know, maybe create MyMoneyObject wit nonempty ID.

kmymoney/mymoney/mymoneyobject_p.h
3

It'll be all deleted in near future anyways.

kmymoney/mymoney/mymoneypayee.h
47–48

MyMoneyPayeeIdentifierContainer doesn't have d_ptr, so no ambiguity here.

kmymoney/mymoney/mymoneytag.h
42–43

MyMoneyTag inherits only MyMoneyObject, so no abiguity when it comes to d_ptr.

wojnilowicz updated this revision to Diff 23280.Dec 2 2017, 2:54 PM
wojnilowicz marked 4 inline comments as done.

Corrected patch according to suggestions and removed copy/move ability of MyMoneyObject.

tbaumgart added inline comments.Dec 2 2017, 3:37 PM
kmymoney/kmymoney.cpp
11

Ah, I forgot to mention, that I saw more of these. Sorry. I scanned real quick and did not see only this one.

kmymoney/mymoney/storage/tests/mymoneydatabasemgr-test.cpp
21

Same as above: but this one is all over the place. If you want to replace it, go ahead. If not: it is not mission critical.

Hi! Did you do a performance comparison? I am concerned that this patch can reduce performance significantly because some of the objects are created very often and in huge amounts. Additionally we perform searches with them. Also I am not sure if these very simple objects (often POD) can really benefit from a d-pointer, except ABI stability. The drawback is increased code complexity.

wojnilowicz abandoned this revision.Dec 3 2017, 4:42 PM

Hi! Did you do a performance comparison? I am concerned that this patch can reduce performance significantly because some of the objects are created very often and in huge amounts. Additionally we perform searches with them. Also I am not sure if these very simple objects (often POD) can really benefit from a d-pointer, except ABI stability. The drawback is increased code complexity.

Hi!

  1. Performance is the least I care for now. I welcome any standarized test procedure and load case for measuring performance.

As of today KMM isn't optimized neither for speed nor compilation time. My goal is to at least reduce compilation time and number of issues reported here http://ebn.kde.org/krazy/reports/extragear/office/kmymoney/index.html.

  1. QString in MyMoneyObject isn't POD type
  2. KMM is already complex in some areas (see reports) and it gives us nothing. Complexity here gives us, besides ABI stability, hopefully reduced compilation time.

If you would look a little bit closer, d-pointer in MyMoneyObject is shared and used in all classes that inherit from it. That classes have more variables in them and IMO that already justifies d-pointer in MyMoneyObject.