c++/boost/gnc_numeric question

John Ralls jralls at ceridwen.us
Sat Jan 14 13:25:57 EST 2017


> On Jan 14, 2017, at 3:54 AM, Geert Janssens <geert.gnucash at kobaltwit.be> wrote:
> 
> Hi,
> 
> The last few days I have payed some attention to our Travis-CI integration. Good news is we can 
> now run our unit tests based on google test (thanks to John as well for some extra fixes).
> 
> The bad news is my csv-import work fails to build on travis while it builds just fine on my local 
> system. The most recent build failure can be seen here:
> 
> https://travis-ci.org/gjanssens/gnucash/builds/191881753
> 
> This has been failing for a while, even before fixing the gtest stuff. My investigation seems to 
> suggest the issues stems from using a different version of boost on my system (1.60.0) versus 
> the one used on travis (1.54.0).
> 
> The failure is in this line:
>    if ((m_deposit == boost::none) &&
>        (m_withdrawal == boost::none))
> with both m_deposit and m_withdrawal being of type
> boost::optional<gnc_numeric>.
> It appears boost 1.54.0 doesn't know how to run this comparison and the boost::optional 
> revision history suggests this was addressed in version 1.56.0 [1].
> 
> Since our boost baseline is still 1.53.0, I'm looking for ways to handle this differently. 
> gnc_numeric is a C struct. My csv-importer work however is all in c++, so why not use the 
> underlying c++ code of gnc_numeric directly, right ?
> 
> So I am working my way through this and have some questions about this.
> 
> 1. I assumed the C++ equivalent of gnc_numeric would be GncNumeric. However this appears 
> to be wrong assumption. GncNumeric is only defined as an alias inside gnc-numeric.cpp and 
> hence not exposed at all to the outside world. Is new c++ code not supposed to use 
> GncNumeric ? Should GncRational be used directly instead ?
> 
> 2. I continued with GncRational for now. However this seems to implement only fairly limited 
> subset of the functions in gnc_numeric. For example there is gnc_numeric_mul and 
> GncRational::mul. The former takes two extra parameters (one for denom preference and one to 
> tell how to round). GncRational::mul on the other hand takes a denom object as second 
> parameter which encapsulates all that information in addition to the two objects we're about to 
> multiply. I'm not sure how I should use this. Should each consumer of the GncRational class 
> each time build the denom object itself and then call the GncRational mutators ? That looks like 
> a lot of code overhead. How about an overload for these mutators that takes the denom and 
> round type directly as gnc_numeric_mul does ? Or is there another way to use this I didn't figure 
> out yet ?
> 
> 3. While studying the GncRational code I noted it has doesn't specify the pure constructor 
> "GncRational()", instead leaving this up to the compiler to generate if necessary. That compiler 
> default would create a GncRational with m_num = 0, m_denom = 0 and m_error = 
> GNC_ERROR_OK. I would consider
> m_num = 0, m_denom = 1 and m_error = GNC_ERROR_OK a better default value for 
> GncRational. A particular reason not to implement that ?

Geert,

Thanks for the (somewhat belated) code review! 

Yes, GncRational is at this point incomplete. I did the minimum I could to get rid of the computational limitations of gnc_numeric and left a lot of functionality to be implemented later as needed on the grounds that gnc_numeric has what appears to be duplicated interface. My plan is to use GncRational in new C++ code and implement addtional functions as needed; it wasn't my plan that you'd be the first to be doing that.

GncDenom exists to separate the rounding action from GncRational. Looking at it again after a year I see that I should make round() a GncDenom member function, which would allow templating it and so getting rid of the switch and permitting faster compile-time dispatch.

A second flaw that I see now is that the arithmetic functions shouldn't be doing the rounding; that would allow replacing them with the full set of operators, a far more natural interface *and* better practice because it permits postponing the rounding operation to the end of a chain of calculations.

The reason I exposed GncDenom outside of GncRational was that it seems to be common in usage to have a bunch of calculations using the same rounding parameters, so the idea was that one could create a single GncDenom and pass it around to all of the operations. C++ overloading makes it easy to have that cake and to eat it by providing 
template <class GD> GncRational::round(const GD&) (where GD is a specialization on RoundType) and GncRational::round(DenomType, RoundType) which creates the GncDenom and calls the template version. 

You're absolutely right about the default constructor.

Regards,
John Ralls




More information about the gnucash-devel mailing list