<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120815/">https://git.reviewboard.kde.org/r/120815/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 2nd, 2014, 8:24 p.m. UTC, <b>Christian David</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The patch looks good and I like it. It also works on my computer :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also I think the change <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">mpq_class &valueRef() const</code> to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">const mpq_class &valueRef() const</code> is not ABI compatible.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If it is not ABI compatible the so version must be increased.</p></pre>
</blockquote>
<p>On November 3rd, 2014, 5:13 a.m. UTC, <b>Cristian Oneț</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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?</p></pre>
</blockquote>
<p>On November 4th, 2014, 7:40 a.m. UTC, <b>Christian David</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This depends on the policy about ABI changes in alkimia. It should be enough to set <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">set(ALKIMIA_LIB_SOVERSION "${VERSION_MAJOR}")</code> to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">set(ALKIMIA_LIB_SOVERSION "5")</code>. This will prevent that an application linked to alkimia starts with an unsupported ABI version.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But if alkimia guarantees ABI compatibility within a major release, the version has to be increased to 5.0.0.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On November 4th, 2014, 9:33 a.m. UTC, <b>Cristian Oneț</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On November 4th, 2014, 6:42 p.m. UTC, <b>Christian David</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">set(ALKIMIA_LIB_SOVERSION "5")</code> 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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">const mpq_class &valueRef() const</code> public, add another method <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">const mpq_class& toMpqClass() const</code> or should I add two methods (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">long numerator() const</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">long denominator() const</code>)?</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thinking about this I'm not sure that gmpxx.h should be even exposed by alkvalue.h at all. Could you give a bit more detail where do you need the numerator and denominator? I would go with some forwad declarations and including gmpxx.h only where it's needed. Right now it ends up in a lot of places because it's included in alkvalue.h.</p></pre>
<br />
<p>- Cristian</p>
<br />
<p>On October 26th, 2014, 9:33 p.m. UTC, Cristian Oneț wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KMymoney and Thomas Baumgart.</div>
<div>By Cristian Oneț.</div>
<p style="color: grey;"><i>Updated Oct. 26, 2014, 9:33 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
alkimia
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Before this the biggest cost of using an AlkValue object, implicitly a KMyMoneyMoney object was assignment, construction and destruction.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">By using implicit sharing combined with a shared zero value, copying on assignment and construction can be greatly reduced.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Implicit sharing was easy to implement using QSharedDataPointer.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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).</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>libalkimia/CMakeLists.txt <span style="color: grey">(3dbe4db)</span></li>
<li>libalkimia/alkvalue.h <span style="color: grey">(7c02403)</span></li>
<li>libalkimia/alkvalue.cpp <span style="color: grey">(ae5d3a4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120815/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="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.3.2</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/ceb1cb24-fe5e-414e-a0c1-09746bcb9bcc__alk-value-implicitly-shared.png">Callgrind data using AlkValue from alkimia 4.4.0 (with implicit sharing)</a></li>
<li><a href="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 file by KMyMoney using review 120818 and alkimia 4.3.2</a></li>
<li><a href="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.3.2</a></li>
<li><a href="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 loading register by KMyMoney using review 120818 and alkimia 4.4.0</a></li>
<li><a href="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.3.2</a></li>
<li><a href="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 editing transaction by KMyMoney using review 120818 and alkimia 4.4.0</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/10/26/8808ebc3-a112-4425-b35e-05acd8ba94d1__alk-value-4.4.0.png">Callgrind data while loading file by KMyMoney using review 120818 and alkimia 4.4.0</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>