Bug 145614 - Convert #define to enum or constexpr
Summary: Convert #define to enum or constexpr
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
Keywords: difficultyBeginner, easyHack, skillCpp
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2021-11-10 12:30 UTC by Hossein
Modified: 2022-04-04 10:48 UTC (History)
3 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-10 12:30:43 UTC
In some older parts of the code, there are several consecutive symbolic constants that are #defined, and possibly have similar prefixes. You can improve this by replacing them with:

a) 'enum class' or 'enum': where facing multiple values of the same category or with similar prefixes. Some of these enums come from standards or other external documents or sources. It is good to find the source, and cross-check the values with it.
'enum class' is preferred to 'enum', but enums with binary or comparison operations can not be easily converted to 'enum class'.

b) 'constexpr': where there is a single value

For example, in emfio/inc/mtftools.hxx:

/* Mapping modes */
#define MM_TEXT                 1
#define MM_LOMETRIC             2
#define MM_HIMETRIC             3
#define MM_LOENGLISH            4
#define MM_HIENGLISH            5
#define MM_TWIPS                6
#define MM_ISOTROPIC            7
#define MM_ANISOTROPIC          8

#define LF_FACESIZE             32

is converted to:

/* Mapping modes */
enum MappingMode
{
    MM_TEXT        = 0x01,
    MM_LOMETRIC    = 0x02,
    MM_HIMETRIC    = 0x03,
    MM_LOENGLISH   = 0x04,
    MM_HIENGLISH   = 0x05,
    MM_TWIPS       = 0x06,
    MM_ISOTROPIC   = 0x07,
    MM_ANISOTROPIC = 0x08
};

constexpr sal_Int32 LF_FACESIZE = 32;
Comment 1 Stephan Bergmann 2021-11-16 14:54:52 UTC
(In reply to Hossein from comment #0)
> /* Mapping modes */
> enum MappingMode
> {
>     MM_TEXT        = 0x01,
>     MM_LOMETRIC    = 0x02,
>     MM_HIMETRIC    = 0x03,
>     MM_LOENGLISH   = 0x04,
>     MM_HIENGLISH   = 0x05,
>     MM_TWIPS       = 0x06,
>     MM_ISOTROPIC   = 0x07,
>     MM_ANISOTROPIC = 0x08
> };

Why change the values from decimal to hexadecimal?

> constexpr sal_Int32 LF_FACESIZE = 32;

I guess some explanation is needed what type to use (`sal_Int32` will not be the best choice in general; even `auto` may be an appropriate choice).
Comment 2 Hossein 2021-11-16 15:36:14 UTC
(In reply to Stephan Bergmann from comment #1)
> (In reply to Hossein from comment #0)
> > /* Mapping modes */
> > enum MappingMode
> > {
> >     MM_TEXT        = 0x01,
> >     MM_LOMETRIC    = 0x02,
> >     MM_HIMETRIC    = 0x03,
> >     MM_LOENGLISH   = 0x04,
> >     MM_HIENGLISH   = 0x05,
> >     MM_TWIPS       = 0x06,
> >     MM_ISOTROPIC   = 0x07,
> >     MM_ANISOTROPIC = 0x08
> > };
> 
> Why change the values from decimal to hexadecimal?

That's the way they are defined in [MS-WMF] documentation:

https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-wmf/97b31821-a0db-4113-a210-fffbcedcec4a

In some enumerations in the same file, I had to add some values that were missing. So, when the source of the enum definition is known, I think using the same definitions can help avoid mistakes. If possible, using the same name would also be good.

> > constexpr sal_Int32 LF_FACESIZE = 32;
> 
> I guess some explanation is needed what type to use (`sal_Int32` will not be
> the best choice in general; even `auto` may be an appropriate choice).

Correct. I will add some explanation for this.
Comment 3 Kevin Suo 2021-11-17 11:29:37 UTC
Set to New as this is an easy hack.
Comment 4 Commit Notification 2021-12-07 15:26:23 UTC
Pesi Taototo committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/47503fce3ebc0874534175c0d9ea40d3a5bbffde

tdf#145614 Convert #define to 'enum class'

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 5 Commit Notification 2022-01-19 05:56:06 UTC
VaibhavMalik4187 committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/40bab1e31c7865f8c45883b8e4b684c0134b9191

tdf#145614 Convert #define to enum or constexpr

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 6 Commit Notification 2022-01-28 11:23:47 UTC
Ramreiso Kashung committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/124059f64a87d8074a0be32d6cc5f74c71bf836e

tdf#145614: Convert #define to enum

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 7 Commit Notification 2022-03-15 12:01:39 UTC
Hossein committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/3e7dd04dd8ca1baea4b7918eb7a7080c595c4625

tdf#145614 Convert #define to enum and constexpr

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 8 Commit Notification 2022-03-18 15:31:33 UTC
Deep17 committed a patch related to this issue.
It has been pushed to "master":

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

tdf#145614 Convert #define to enum or constexpr

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-03-22 07:38:04 UTC
Deep17 committed a patch related to this issue.
It has been pushed to "master":

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

tdf#145614 Convert #define to enum or constexpr

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-03-29 11:28:28 UTC
offtkp committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/64888968b30fb387d8c2536664117915e5423b0c

tdf#145614 Convert #define to constexpr

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.