Bug 81356 - replace use of Fraction by boost::rational
Summary: replace use of Fraction by boost::rational
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Juan Picca
URL:
Whiteboard: target:4.4.0 target:4.4.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2014-07-14 17:49 UTC by David Tardon
Modified: 2015-12-16 00:09 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Tardon 2014-07-14 17:49:21 UTC
The use of the old Fraction class from tools module should be replaced by boost::rational.
Comment 1 Marcin 2014-07-18 15:10:36 UTC
Hi,
I want to take care of this hack, additionally this would be my first EasyHack.So before I start I wan to ask does successfull compilation can be use as definition of done?
Comment 2 Marcin 2014-07-18 15:15:33 UTC
Ok I have found error in my question. compilation result can't be used as DoD
Comment 3 Juan Picca 2014-08-04 01:12:46 UTC
Hi,
Some questions about the task:

1. Fraction uses internally a long for hold the numerator and the denominator. boost::rational uses a template based implementation. How replace Fraction?

a. using boost::rational
   ...
   later, replace Fraction with rational<long>
b. replace Fraction with boost::rational<long>
c. typedef boost::rational<long> Rational;
   ...
   later, replace Fraction with Rational

a. and b. has the cons that in the future, by mistake, someone can write rational<long long> or rational<int>. The pro is not add a new type.
 
2. the constructor Fraction(double) has no equivalent in boost::rational. We can create a helper function
    boost::rational<long> create_rational(double);
for do the creation of a rational instance (avoiding narrowing)? Where?

3. what is the equivalent of Fraction::ReduceInaccurate(unsigned) in boost::rational?
Comment 4 David Tardon 2014-08-04 10:22:56 UTC
(In reply to comment #3)
> Hi,
> Some questions about the task:
> 
> 1. Fraction uses internally a long for hold the numerator and the
> denominator. boost::rational uses a template based implementation. How
> replace Fraction?
> 
> b. replace Fraction with boost::rational<long>

That one. It could even be boost::rational<int>, as the current Fraction can only correctly handle 32-bit values anyway (ReadFraction, WriteFraction).

> 2. the constructor Fraction(double) has no equivalent in boost::rational. We
> can create a helper function
>     boost::rational<long> create_rational(double);
> for do the creation of a rational instance (avoiding narrowing)? Where?

That depends at how many places call that ctor. If it is only one, the best thing is to move the code there. Otherwise, it must remain in tools, as the implementation uses BigInt. I think reusing the original fract.hxx header for this is okay. We do not have to change more than necessary. But I would accept creating a new header include/tools/rational.hxx (and correspondingly named cxx file). 

> 
> 3. what is the equivalent of Fraction::ReduceInaccurate(unsigned) in
> boost::rational?

There is no equivalent. You have to provide a standalone function (template) for this in include/tools/fract.hxx (which would use the old code, of course. There is no need to reimplement this from scratch). It is your call if you let the function to modify its argument or to return a new value instead. I.e., either 

void reduceInnacurate(boost::rational<long> &, unsigned)

or

boost::rational<long> reduceInnacurate(const boost::rational<long> &, unsigned)

is fine. It would be good to check the current use: if ReduceInaccurate is more often called on a copy of a fraction, then the latter would be preferred.

4. ReadFraction/WriteFraction are only used in vcl/source/gdi/mapmod.cxx, so they should be moved there.

5. Code that relies on operator double() or operator long() should be changed to use rational_cast.
Comment 5 Juan Picca 2014-08-18 16:01:42 UTC
I add a draft for a rewrite plugin: https://gerrit.libreoffice.org/11000
Comments are welcome.
Comment 6 Juan Picca 2014-09-11 18:52:38 UTC
The previous draft was abandoned because i published it by error when was not ready.
I create a new draft based in the draft 11000: https://gerrit.libreoffice.org/11405

Reading in the documentation i found that boost::rational don't allow create an invalid value, it throws a boost::bad_rational exception.
In the other side Fraction allow us to create an invalid value and use IsValid() method for check it.

Questions: 
1. Can we check if inside function ReadFraction (include/tools/fract.hxx - tools/source/generic/fract.cxx, see the draft) we are reading an invalid value and instead of throw an exception return the rational zero?

2. In the following files the Fraction.IsValid() method is used:
svx/source/sdr/properties/itemsettools.cxx
svx/source/svdraw/svdattr.cxx
svx/source/svdraw/svdomeas.cxx
sw/source/core/txtnode/fntcache.cxx
sw/source/uibase/uiview/viewport.cxx
Can we take the usage and instantiation of Fraction in the rest of the code as valid and avoid catching bad_rational exceptions when we create a rational instance?
Comment 7 Juan Picca 2014-09-19 22:01:37 UTC
What about rewrite the Fraction class as a wrapper to boost::rational<long> class?
The motivation is for allow doing the refactor incrementally.

Proposed change:

class Fraction {
private:
  boost::rational<long> *value;
public:
  Fraction(boost::rational<long> *rational) { value = rational;}
  boost::rational<long> *GetReference() { return value;}

  // refactor code using pointer 'value'
}

For code that breaks the interface during the refactor we can use this change:

void function_that_break_interface(boost::rational<long>& converted_param1,
                                   boost::rational<long> converted_param2) {
  Function param1(&converted_param1);
  Function param2(&converted_param2);
    
  ....
}
Comment 8 Juan Picca 2014-10-03 11:54:33 UTC
Added a work in progress: https://gerrit.libreoffice.org/11551/
Currently a problem is how represent an invalid rational.
boost::rational only allow create instances of "valid" rationals, that is, denominator need be distinct from 0.
In ReadFraction (/tools/source/generic/rational.cxx) i choose treat invalid rationals as zero rational, but some tests fails.
Some advice or strategy for treat this globally?
Comment 9 Stefan Weiberg 2014-10-22 14:58:11 UTC
Unfortunately the commit 47a2d7642d249d70b5da0c330a73f3a0032e4bba is the first bad commit in my bisect search for a current bug in our LibreOffice scalc build (Ubuntu 12.04 32bit LHM).

Bisect Log and Report:
http://pastebin.com/tW8MU60f

The following error occurs:
http://imgur.com/kHKVBw5

There are no cells displayed in scalc. In addition "marked" cells turn black.
Comment 10 Commit Notification 2014-10-28 17:16:15 UTC
Juan Picca committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2ce0aededea43231d91a0955fc0676120dcc4f13

fdo#81356: use boost::rational internally in Fraction

It will be available in 4.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 11 Robinson Tryon (qubit) 2015-12-16 00:09:50 UTC
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup )
[NinjaEdit]