The use of the old Fraction class from tools module should be replaced by boost::rational.
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?
Ok I have found error in my question. compilation result can't be used as DoD
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?
(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.
I add a draft for a rewrite plugin: https://gerrit.libreoffice.org/11000 Comments are welcome.
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?
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); .... }
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?
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.
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.
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup ) [NinjaEdit]