Bug 161837 - Drop Fraction class from tools
Summary: Drop Fraction class from tools
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyMedium, easyHack, skillCpp, topicCleanup
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2024-06-29 09:23 UTC by Mike Kaganski
Modified: 2024-11-07 05:17 UTC (History)
3 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 Mike Kaganski 2024-06-29 09:23:34 UTC
In the deprecated tools library, there is a Fraction class (<tools/fract.hxx>), that is used rather extensively, e.g. in VCL.

It has problems, including performance: e.g. it is built using boost/rational, but to avoid dependency of each library using the class on boost, it's used in the tools' CXX, so no inlining. But the most problematic is its unreliable nature. It has a 'mbValid' member, which may be set to false on an intermediate overflow; and then, the invalid fraction breaks all calculations.

Consider a valid fraction with numerator 72863296 and denominator 92331675, approximately equal to 0.7891. Trying to multiply number 193 by this fraction will give 0:

    Fraction f(72'863'296, 92'331'675);
    assert(f.IsValid());
    auto x = (Fraction(193) * f).operator sal_Int32();

Here, x is suppose to be 152, but will be 0, because multiplying 193 by 72'863'296 gives 14'062'616'128, which overflows the underlying sal_Int32, setting the multiplication result to invalid (see Fraction::operator *= in tools/source/generic/fract.cxx).

Of course, it is possible to fix such things one by one, by introducing more checks, branching the code to handle different cases, and hoping that now it will give no surprises, until a next case is found. But these days, floating point calculations are not much slower than integer; and given the lack of inlining, rather complex logic and fragility, the use of Fraction seems to slow down, introduce errors, and give no upsides at all.

This task is to replace all uses of Fraction in the codebase by doubles; and in the end, drop the Fraction code from tools. This is a task that implies many developers, so should not be assigned to yourself.
Comment 1 Hossein 2024-07-10 14:04:34 UTC
Assigning the issue to myself as per discussion on IRC. I am trying to do this change in one round, or at least do the VCL part in that. This is what I have done, which is not complete yet:

tdf#161837 Use double instead of Fraction class
https://gerrit.libreoffice.org/c/core/+/170084
Comment 2 Buovjaga 2024-11-07 05:17:41 UTC
Per the discussion in the patch, looks like this should not be assigned anymore, but done in smaller bits.