Bug 145539 - Convert string literals defined as const char[] in header files to constexpr OUStringLiteral
Summary: Convert string literals defined as const char[] in header files to constexpr ...
Status: RESOLVED FIXED
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.3.0 reviewed:2023 target:24.8.0
Keywords: difficultyBeginner, easyHack, skillCpp
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2021-11-04 11:26 UTC by Hossein
Modified: 2024-02-14 09:10 UTC (History)
5 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 2021-11-04 11:26:07 UTC
There are several string literals in the header files, defined in various ways like const char[]. You should convert string literals in header files from "const char[]" to "constexpr OUStringLiteral".

Instances of the literals defined as "const char[]" can be found using:

git grep "^const char" *.hxx|grep -F [

A sample can be found here:
https://git.libreoffice.org/core/+/8fb366c13ac1b23c455c32afc085bca2edff03bb
Comment 1 Po-Yen Huang 2021-11-05 02:57:19 UTC
I pushed a patch to gerrit: https://gerrit.libreoffice.org/c/core/+/124724
Please review it :-)
Comment 2 Commit Notification 2021-11-25 14:59:00 UTC
Jeff Huang committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/7b0e0409c3f24dfe37d9c43ffa439b56ea719868

tdf#145539 const OUString -> constexpr OUStringLiteral

It will be available in 7.3.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 Po-Yen Huang 2021-12-24 02:51:13 UTC
When I use 'git grep "^const char" *.hxx| grep -F [', I can only find this place, so this issue should be closed when 7.3.0 is out I think?
Comment 4 Xisco Faulí 2022-05-02 14:50:30 UTC
(In reply to Po-Yen Huang from comment #3)
> When I use 'git grep "^const char" *.hxx| grep -F [', I can only find this
> place, so this issue should be closed when 7.3.0 is out I think?

Should this issue be closed as RESOLVED FIXED now ?
Comment 5 Po-Yen Huang 2022-05-03 01:10:19 UTC
(In reply to Xisco Faulí from comment #4)
> (In reply to Po-Yen Huang from comment #3)
> > When I use 'git grep "^const char" *.hxx| grep -F [', I can only find this
> > place, so this issue should be closed when 7.3.0 is out I think?
> 
> Should this issue be closed as RESOLVED FIXED now ?

I think yes, if no other one find another string for this.
Comment 6 Buovjaga 2022-05-03 05:22:34 UTC
What about all the ones where const char is not immediately at the beginning of the line:

git grep "const char" *.hxx| grep -F [
Comment 7 Po-Yen Huang 2022-05-03 05:44:46 UTC
(In reply to Buovjaga from comment #6)
> What about all the ones where const char is not immediately at the beginning
> of the line:
> 
> git grep "const char" *.hxx| grep -F [

Ah yeah, there are some strings when using this command, should they be all replaced?
Comment 8 Buovjaga 2022-05-03 06:00:46 UTC
(In reply to Po-Yen Huang from comment #7)
> (In reply to Buovjaga from comment #6)
> > What about all the ones where const char is not immediately at the beginning
> > of the line:
> > 
> > git grep "const char" *.hxx| grep -F [
> 
> Ah yeah, there are some strings when using this command, should they be all
> replaced?

Let's wait for Hossein's opinion (he comes back in a couple of days)
Comment 9 Hossein 2023-07-24 12:34:01 UTC
Re-evaluating the EasyHack in 2023.

This issue is still relevant.

For header files, nothing is left as the remaining instances are all inside sc/source/core/opencl, and can be safely ignored.

git grep "^const char" *.hxx|grep -F [

But, there are still remaining instances in the C++ source files:

git grep "^const char" *.cxx|grep -F [

So, let's keep this issue open, and fix the remaining instances in the source files. Please note that not every instance is suitable for this change.

For example, there are some instances remaining in this file, that are suitable for the change:

xmlhelp/source/treeview/tvread.cxx
Comment 10 Devansh Varshney 2024-02-01 18:13:42 UTC
do these in the file https://github.com/LibreOffice/core/blob/c3d00f4ca0ec5a53d694ab9924fd79d0e536aec4/xmlhelp/source/treeview/tvread.cxx#L160C1-L164C46

are eligible for change somewhat like -

constexpr OUStringLiteral prodName = u"%PRODUCTNAME";

but for this I am getting -

expression must have a constant valueC/C++(28)
tvread.cxx(160, 38): invalid type "<error-type>" for constant-expression evaluation
(const char16_t [13])u"%PRODUCTNAME"

One more thing does the below file also needs the same changes?

https://github.com/LibreOffice/core/blob/c3d00f4ca0ec5a53d694ab9924fd79d0e536aec4/sc/source/core/opencl/op_math_helpers.hxx#L4

This is what I did to the first const char[] - 

inline constexpr OUStringLiteral Math_IntgDecl =u"double Intg(double n);\n";

I am asking for this as in the comment 9 it's written that- can be safely ignored.
Comment 11 Devansh Varshney 2024-02-02 09:17:12 UTC
(In reply to Devansh Varshney from comment #10)
> do these in the file
> https://github.com/LibreOffice/core/blob/
> c3d00f4ca0ec5a53d694ab9924fd79d0e536aec4/xmlhelp/source/treeview/tvread.
> cxx#L160C1-L164C46
> 
> are eligible for change somewhat like -
> 
> constexpr OUStringLiteral prodName = u"%PRODUCTNAME";
> 
> but for this I am getting -
> 
> expression must have a constant valueC/C++(28)
> tvread.cxx(160, 38): invalid type "<error-type>" for constant-expression
> evaluation
> (const char16_t [13])u"%PRODUCTNAME"
> 
> One more thing does the below file also needs the same changes?
> 
> https://github.com/LibreOffice/core/blob/
> c3d00f4ca0ec5a53d694ab9924fd79d0e536aec4/sc/source/core/opencl/
> op_math_helpers.hxx#L4
> 
> This is what I did to the first const char[] - 
> 
> inline constexpr OUStringLiteral Math_IntgDecl =u"double Intg(double n);\n";
> 
> I am asking for this as in the comment 9 it's written that- can be safely
> ignored.

In libreoffice/core/sw/inc/calc.hxx we have -

extern const char sCalc_Add[];

do we also need to change these definitions as we have these used in sw/source/core/bastyp/calc.cxx
Comment 12 Buovjaga 2024-02-02 17:13:36 UTC
(In reply to Devansh Varshney from comment #10)
> One more thing does the below file also needs the same changes?
> 
> https://github.com/LibreOffice/core/blob/
> c3d00f4ca0ec5a53d694ab9924fd79d0e536aec4/sc/source/core/opencl/
> op_math_helpers.hxx#L4
> 
> This is what I did to the first const char[] - 
> 
> inline constexpr OUStringLiteral Math_IntgDecl =u"double Intg(double n);\n";
> 
> I am asking for this as in the comment 9 it's written that- can be safely
> ignored.

Comment 9 says to ignore it, so don't change it.
Comment 13 Commit Notification 2024-02-14 08:59:20 UTC
varshneydevansh committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/0859e497be07acceb0a950183d5997374783266f

tdf#145539 convert const char[] in files to constexpr OUStringLiteral

It will be available in 24.8.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 14 Stephan Bergmann 2024-02-14 09:10:51 UTC
With the recent introduction of O[U]String literals ("..."_ostr, u"..."_ustr), there should generally be no more need for O[U]StringLiteral, and the overall plan is to eventually get rid of O[U]StringLiteral.  For any given use of string literals, how best to represent them in the code depends on how exactly they are used.  That might be O[U]String literals, or std::[u16]string_view.  Various loplugin compiler plugins are meant to warn if the code uses a sub-optimal representation.  Lets better close this easy hack now and rely on compiler plugins going forward to clean up those string literal uses.