Bug 145759 - Use symbolic constants instead of magic numerical constants
Summary: Use symbolic constants instead of magic numerical constants
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: target:7.4.0 reviewed:2022
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2021-11-18 14:48 UTC by Hossein
Modified: 2022-06-16 13:46 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Hossein 2021-11-18 14:48:11 UTC
Using a numerical literal directly in the code can cause problems. Because of this, it is suggested that it should be replaced by some numerical constant.

ES.45: Avoid "magic constants"; use symbolic constants
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-magic

If it is known (like π), you should use the appropriate symbolic constant like M_PI. If not, you should define a new constant with proper name and type with 'contsexpr'.

One solution to find such magic constants is to start from a list of some well known mathematical constants:
https://en.wikipedia.org/wiki/List_of_mathematical_constants

Then, store some of them in a text file, let's say 'constants.txt', then search for all these values inside CPP files:

git grep -Ff constants.txt *.cxx *.hxx

Many of these symbolic constants like M_PI are already defined in C++ standard library or some place in the LibreOffice code, and you can use them easily.

You should examine the 'grep' results carefully, because not every 3.14 refers to PI.
Comment 1 Xisco Faulí 2021-11-23 10:04:24 UTC
Moving to NEW
Comment 2 Somya Srivastava 2021-11-23 15:22:25 UTC
Hi! I would like to work on this issue.
Comment 3 Buovjaga 2021-12-01 11:10:28 UTC
Multi-hack, no need to assign
Comment 4 Libreoffice user SSO 2022-01-22 11:23:40 UTC
I tried suggested approach in the description. but the command "git grep -n -Ff constants.txt *.cxx *.hxx" or "git grep -Ff constants.txt *.cxx *.hxx" is not generating any output. So, my conclusion is that no constants from given wikipedia article are used in any c++ files.

My constants.txt contains constants in following pattern:
const-name <constant>
.
.
.
Comment 5 Buovjaga 2022-01-22 12:06:05 UTC
(In reply to Libreoffice user SSO from comment #4)
> I tried suggested approach in the description. but the command "git grep -n
> -Ff constants.txt *.cxx *.hxx" or "git grep -Ff constants.txt *.cxx *.hxx"
> is not generating any output. So, my conclusion is that no constants from
> given wikipedia article are used in any c++ files.
> 
> My constants.txt contains constants in following pattern:
> const-name <constant>
> .
> .
> .

Don't use such a pattern, just have one value per line :)

3.14159
1.41421

and so forth
Comment 6 Kunal Pawar 2022-02-06 13:07:31 UTC
Where should be define the variables, especially in case of multiple values in same file?
Comment 7 Kunal Pawar 2022-02-10 08:09:11 UTC
I have tried updating some of the constants.
https://gerrit.libreoffice.org/c/core/+/129745
Comment 8 Commit Notification 2022-02-15 11:18:52 UTC
pragat-pandya committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/5f21eaa0e6b689233336cacc949b9a55d545088f

tdf#145759 Using M_PI from cmath instead of magic constants.

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 9 Commit Notification 2022-02-15 20:01:04 UTC
Kunal Pawar committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/49746f40b06eaf1f61bb54454408a06a49d73c5e

tdf#145759 Use symbolic constants instead of magic numerical constants

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 10 Commit Notification 2022-02-25 18:13:00 UTC
Hossein committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/0308e48e46cee2f56a6239c8479d26185146d74a

tdf#145759 30.6001 -> monthDaysWithoutJanFeb

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 11 Hossein 2022-05-26 11:01:58 UTC
@Ehsan:
This is a multi-hacker EasyHack. You can work on it without assigning it to yourself.
Comment 12 Hossein 2022-06-16 13:46:36 UTC
Re-evaluating the EasyHack in 2022

This enhancement to the code is still relevant. One may find more places in the code where symbolic constants can be used instead of magic numerical constants.