Bug 148430 - Use std functions instead of our own implementation
Summary: Use std functions instead of our own implementation
Status: RESOLVED WONTFIX
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: target:7.4.0 reviewed:2022 target:7.5.0
Keywords:
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2022-04-06 20:16 UTC by Hossein
Modified: 2022-08-23 06:45 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 Hossein 2022-04-06 20:16:43 UTC
Historically, some functions including various mathematical functions were not available in the early versions of C/C++. In order to use those functions inside LibreOffice, these functions were implemented internally.

For an example, some (inverse) trigonometric and (inverse) hyperbolic functions like asinh() and acosh() were not available in the early versions of C++ std.

Instead of our own implementation inside LibreOffice, we now can use std::asinh() and std::acosh() from <cmath>, available since C++11:

    https://en.cppreference.com/w/cpp/numeric/math/asinh

The underlying methods from <math.h> are available since C99:

    https://en.cppreference.com/w/c/numeric/math/asinh

The reason provided in f70de5267d7d9b7b6946cd72fe26e91bb6ac8431 to provide an internal implementation was that asinh() and acosh() were "part of the C99 standard, but not provided by some compilers". This was true at that time, but is no longer the case as the methods are now well established.
Comment 1 Roman Kuznetsov 2022-04-06 21:22:24 UTC
Set to NEW
Comment 2 Commit Notification 2022-05-11 08:34:08 UTC
offtkp committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/6f75ec6115f0152517be634070607bc61bf96dd0

tdf#148430 Use atanh from <cmath> instead of our own

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 3 Hossein 2022-06-16 14:09:05 UTC
Re-evaluating the EasyHack in 2022

This enhancement is still relevant.
Comment 4 Eike Rathke 2022-08-15 13:51:38 UTC
Do NOT replace rtl::math::sin(), cos() and tan() with their std::... counterparts. Note that the rtl implementations check the argument with isValidArcArg(), see that comment in include/rtl/math.hxx, and if it's out of range set a NaN at the double, which callers rely on (if aware).
Comment 5 Hao Liu 2022-08-17 09:11:16 UTC
I recently tried fixing this bug, and I found more hidden problems here.
https://gerrit.libreoffice.org/c/core/+/138294

The rtl sin/cos/tan implementations are different from std, rtl functions check the argument with isValidArcArg(), so these three functions can't be replaced.

The rtl asin/acos implementations are better than std in some cases according to
https://bz.apache.org/ooo/show_bug.cgi?id=97605, and It can't pass the unit tests on some environments. So these two functions can't be replaced too.

Maybe this bug report is a bit unnecessary? There are not many benefits to replacing rtl math functions considering these trick problems.
Comment 6 Hao Liu 2022-08-17 09:14:39 UTC
(In reply to Hao Liu from comment #5)
> I recently tried fixing this bug, and I found more hidden problems here.
> https://gerrit.libreoffice.org/c/core/+/138294
> 
> The rtl sin/cos/tan implementations are different from std, rtl functions
> check the argument with isValidArcArg(), so these three functions can't be
> replaced.
> 
> The rtl asin/acos implementations are better than std in some cases
> according to
> https://bz.apache.org/ooo/show_bug.cgi?id=97605, and It can't pass the unit
> tests on some environments. So these two functions can't be replaced too.
> 
> Maybe this bug report is a bit unnecessary? There are not many benefits to
> replacing rtl math functions considering these trick problems.

Should be asinh/acosh.
Comment 7 Hossein 2022-08-19 07:28:16 UTC
This issue is discussed in the latest ESC call, as there were concerns on how precise the std implementations of atanh/acosh are on different platforms. The decision was to close the EasyHack.

ESC meeting minutes: 2022-08-18
https://lists.freedesktop.org/archives/libreoffice/2022-August/089274.html

Therefore, I remove the EasyHack tag from this issue, and also close the issue to keep the internal implementation for now. I think this issue can re-opened if we can make sure the precision of the std implementation of these functions are acceptable on different platforms.
Comment 8 Commit Notification 2022-08-19 15:24:40 UTC
Liu Hao committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/466f86abb7208e853ad48ae46e97a2455667fa42

tdf#148430 Use std math functions instead of rtl::math

It will be available in 7.5.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 9 Commit Notification 2022-08-23 06:45:39 UTC
Liu Hao committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/e3d120e2ba991897bf7d8eff6cc4eba00f7d749e

tdf#148430 Use std math functions instead of rtl::math

It will be available in 7.5.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.