> On Nov. 2, 2014, 9:24 nachm., Christian David wrote:
> > The patch looks good and I like it. It also works on my computer :)
> >
> > I am just not totaly sure if this patch is ABI compatible. The size of
> > AlkValue is unchanged but is that enough? After all the QSharedDataPointer
> > has a constructor and destructor, in which compile unit is it called?
> >
> > Also I think the change ```mpq_class &valueRef() const``` to ```const
> > mpq_class &valueRef() const``` is not ABI compatible.
> >
> > If it is not ABI compatible the so version must be increased.
>
> Cristian Oneț wrote:
> Yes, this breaks the ABI. Do you mean that we should change is to 5.0.0?
> Isn't 4.4.0 enough to signal this?
>
> Christian David wrote:
> This depends on the policy about ABI changes in alkimia. It should be
> enough to set ```set(ALKIMIA_LIB_SOVERSION "${VERSION_MAJOR}")``` to
> ```set(ALKIMIA_LIB_SOVERSION "5")```. This will prevent that an application
> linked to alkimia starts with an unsupported ABI version.
>
> But if alkimia guarantees ABI compatibility within a major release, the
> version has to be increased to 5.0.0.
>
> I prefer the first soloution. As far as I know there is no stand alone
> version of alkimia distributed to Mac/OS X or Windows. And on Linux the
> distributions won't have any issues with this (I think). So there is no point
> in keeping ABI compatibility between releases.
>
> Cristian Oneț wrote:
> I'd rather avoid using set(ALKIMIA_LIB_SOVERSION "5") instead I would
> bump the version to 5.0.0. I'll update the patch accordingly.
I think ```set(ALKIMIA_LIB_SOVERSION "5")``` is okay - it is a pure technical
version and likely nobody after us two will ever see it again. But I have to
admit most libraries keep the so version equal to the major version. So your
way of setting the version to 5.0.0 may cause less confusion.
Another question: I need access to the numerator and denominator to convert a
MyMoneyValue to the value type of the online banking library. Should I make
```const mpq_class &valueRef() const``` public, add another method ```const
mpq_class& toMpqClass() const``` or should I add two methods (```long
numerator() const``` and ```long denominator() const```)?
- Christian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120815/#review69687
-----------------------------------------------------------
On Okt. 26, 2014, 10:33 nachm., Cristian Oneț wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120815/
> -----------------------------------------------------------
>
> (Updated Okt. 26, 2014, 10:33 nachm.)
>
>
> Review request for KMymoney and Thomas Baumgart.
>
>
> Repository: alkimia
>
>
> Description
> -------
>
> Before this the biggest cost of using an AlkValue object, implicitly a
> KMyMoneyMoney object was assignment, construction and destruction.
>
> By using implicit sharing combined with a shared zero value, copying on
> assignment and construction can be greatly reduced.
>
> Implicit sharing was easy to implement using QSharedDataPointer.
>
> Bumped the library version because API changes were necessary. The mpq_class
> &valueRef() const method was bad beacause it was returning a non-const
> reference from a const function. Now There is a const version which will not
> detach the shared data while the non-const version will detach from the
> shared data.
>
>
> Diffs
> -----
>
> libalkimia/CMakeLists.txt 3dbe4db
> libalkimia/alkvalue.h 7c02403
> libalkimia/alkvalue.cpp ae5d3a4
>
> Diff: https://git.reviewboard.kde.org/r/120815/diff/
>
>
> Testing
> -------
>
> Succesfully ran KMyMoney and KMyMoney tests. See the attached screeshots for
> the callgrind data about AlkValue obtained while loading the same KMyMoney
> file without and with this change. Note that the second run also contains
> some MyMoneyMoney optimizations based on this feature (subject of a different
> review request).
>
>
> File Attachments
> ----------------
>
> Callgrind data using AlkValue from alkimia 4.3.2
>
> https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/f79cfa4b-b01a-4d14-adbe-dc905ad1a4c9__alk-value.png
> Callgrind data using AlkValue from alkimia 4.4.0 (with implicit sharing)
>
> https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/ceb1cb24-fe5e-414e-a0c1-09746bcb9bcc__alk-value-implicitly-shared.png
> Callgrind data while loading file by KMyMoney using review 120818 and alkimia
> 4.3.2
>
> https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/9d142707-962b-434a-b847-8af7154ff4a1__alk-value-4.3.2.png
> Callgrind data while loading register by KMyMoney using review 120818 and
> alkimia 4.3.2
>
> https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/25200104-be0e-434e-941d-c7c51c9bcb2c__register-load-4.3.2.png
> Callgrind data while loading register by KMyMoney using review 120818 and
> alkimia 4.4.0
>
> https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/58ee7a28-2723-4b2c-9818-e82b0b6a03e8__register-load-4.4.0.png
> Callgrind data while editing transaction by KMyMoney using review 120818 and
> alkimia 4.3.2
>
> https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/6f25d85b-3c3e-4d92-9f1a-1315daf3788c__transaction-edit-4.3.2.png
> Callgrind data while editing transaction by KMyMoney using review 120818 and
> alkimia 4.4.0
>
> https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/e4d5d50f-efb8-4f0b-bed8-23fa1c5db391__transaction-edit-4.4.0.png
> Callgrind data while loading file by KMyMoney using review 120818 and alkimia
> 4.4.0
>
> https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/8808ebc3-a112-4425-b35e-05acd8ba94d1__alk-value-4.4.0.png
>
>
> Thanks,
>
> Cristian Oneț
>
>
_______________________________________________
KMyMoney-devel mailing list
KMyMoney-devel@kde.org
https://mail.kde.org/mailman/listinfo/kmymoney-devel
