All tests pass.
Details
- Reviewers
tbaumgart - Group Reviewers
KMyMoney - Commits
- R261:fd0c2ec455de: Create d-pointer for MyMoneyObject
Diff Detail
- Repository
- R261 KMyMoney
- Lint
Lint Skipped - Unit
Unit Tests Skipped
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. |
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. |
Corrected patch according to suggestions and removed copy/move ability of MyMoneyObject.
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.
Hi!
- 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.
- QString in MyMoneyObject isn't POD type
- 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.