Bug 138109 - Create and use a dedicated Length C++ type
Summary: Create and use a dedicated Length C++ type
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
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2020-11-10 10:20 UTC by Mike Kaganski
Modified: 2021-08-15 05:16 UTC (History)
7 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 2020-11-10 10:20:57 UTC
We use length measures intensively throughout the codebase. We use millimeters, inches, hundreds of mm, cm, m, twips, points, EMUs, and many others. We convert those in all directions.

I suggest to create a dedicated class (in tools?) that would represent generic length; consistently using such class in the code would allow to minimize rounding errors from repeated conversions (there should ideally be only two conversions: when a length is read from a file, and when it's output), and prevent confusion when a number (representing a length) is received from some function, and it's hard to know what units should be used with the number.

It possibly should use some fixed internal unit for length, and some fixed type for the value. It should provide convenience static and member functions for conversion to/from different units, which would also provide overloads for different resulting typed (esp. to differentiate floating-point results vs integer results, with rounding for the latter). It should also provide user-defined literals for simplifying definitions of things like 2_mm or 25_EMU. It needs to allow usual arithmetical operations.

Subject to discussion, I propose to use mm100 as the class' base units, and double as the type for the stored value. That allows to represent any whole number of mm100s up to 2^53 (i.e., up to 90 071 992 547 meters); it will also allow to unambiguously represent any reasonable whole value in EMUs (greater than 20 million meters).

The task would be first to create such a class, and start using it; then gradually make more and more code use it, passing it around, and avoid conversions except in places of input and output.
Comment 1 Tomaz Vajngerl 2020-11-12 09:20:45 UTC
I already worked on something like [1]. The difference to your suggestion is that the base are EMUS (it's what they were created for - common denominator between imperial and metric) stored as int64 (but there is also a double version, still using EMU as the base).
 

[1] https://cgit.freedesktop.org/libreoffice/core/commit/?h=private/tvajngerl/staging&id=21ab2d10b05cae42b465bff3420705360c49a81a
Comment 2 Mike Kaganski 2020-11-12 09:24:34 UTC
(In reply to Tomaz Vajngerl from comment #1)
> I already worked on something like [1].

Oh. I must had overlooked or not knew about that ... and given that that wasn't merged into master, that turned out more problematic that I imagined - so the "easyhack" idea seem wrong here.
Comment 3 Tomaz Vajngerl 2020-11-12 09:34:56 UTC
I was still experimenting with this outside of LO, so I didn't yet push it anywhere, but as you filled a bug, I added it into LO and pushed it to my staging repository.
Comment 4 Mike Kaganski 2020-11-12 09:45:34 UTC
(In reply to Tomaz Vajngerl from comment #3)

Nice! (Unfortunately I can't comment somewhere in gerrit, so commenting here: I suppose that value() there could better be renamed to _emu(), to correspond to other getters there?)
Comment 5 Mike Kaganski 2020-11-12 09:49:21 UTC
(In reply to Mike Kaganski from comment #4)

... and I'd also suggest making ctor taking the number private, and introduce emu() static function constructing it, so that just any creation of the length would explicitly tell which units are used?
Comment 6 Roman Kuznetsov 2021-04-13 11:36:16 UTC
Let's set it as NEW