Bug Hunting Session
Bug 100726 - Improve readability of OUString/OString concatenations
Summary: Improve readability of OUString/OString concatenations
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium enhancement
Assignee: Not Assigned
URL:
Whiteboard: target:5.3.0 target:5.4.0 target:6.0....
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2016-07-01 13:36 UTC by Muhammet Kara
Modified: 2018-07-09 06:13 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 Muhammet Kara 2016-07-01 13:36:42 UTC
Context:
String cleanups sometimes change ugly code to only slightly less ugly code. With the new feature enabled, any string concatenation/creation is simply done as:

OUString s = foo + bar + "baz" + OUString::number( many ) + "whatever";

All the other alternatives, like explicit OUStringBuffer and repeated append() 
should be now worse in all possible aspects. In fact, this should result in 
just one OUString allocation, one data copy for anything and at most one 
length computation, so it should possibly beat even strcpy+strcat, while at 
the same time looking good. (More on https://goo.gl/jsVAwy: )

Task:
Find occurences of consecutive string concatanations with += operator following creation (or assignment) of a new OUString object. And replace those consecutive += assignments with one single assignment like the one in the context above.

Goal:
More readable and efficient code.

Code pointers:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=35de25aac00f9698c5df338bb6399384a71f6c1c
https://cgit.freedesktop.org/libreoffice/core/commit/?id=471ca6690262463c58225b5c8efb74e8fcf5e1ac
https://cgit.freedesktop.org/libreoffice/core/commit/?id=94809ea0d4101679794bb239313c4d73fab30419
Comment 1 jani 2016-07-01 13:58:31 UTC
Would be good to have the actual code pointers, are a grep to find the locations in question.
Comment 2 Muhammet Kara 2016-07-01 14:43:40 UTC
Additional code pointers:

You can search with
 git grep "s[A-z] +="
to find occurrences of OUString concatanations with += operator.

If code looks like this:
 OUString foo(bar);
 foo += bar;
 foo += "baz";
 foo += OUString::number( many );
 foo += "whatever";

Change it to something like:
 OUString foo = bar
              + bar
              + "baz"
              + OUString::number( many )
              + "whatever";

You can also simply search with
 git grep "+="
to get a lot of results, then you need to check if they are OUString concatanations or arithmetic operations.
Comment 3 Commit Notification 2016-07-04 10:25:24 UTC
Arnold Dumas committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=aee49860ee413547e2779dbd622700343ce255b3

tdf#100726: Improve readability of OUString concatenations

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 4 Commit Notification 2016-07-11 07:29:13 UTC
Arnold Dumas committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c0ca75f1af9a94dccfd1011eac8821bb1a316d6b

tdf#100726: Improve readability of OUString concatenations

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 5 Nadith Malinda 2016-07-11 17:29:49 UTC
Is there still the bug is there.or all the places are fully readable and efficient coded...if not i would like to join also to fix this bug
Comment 6 Commit Notification 2016-07-15 08:39:17 UTC
nadith committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=bfe6c2dedbac4caa8a080a7a7b2d783ae5d5d813

tdf#100726: Improve readability of OUString concatenation a bit

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 7 Commit Notification 2016-07-15 08:40:42 UTC
nadith committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=e7d709cb2487dc67179812afa5c36d111d9400be

tdf#100726: Improve readability of OUString concatenations in basctl module

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 8 Commit Notification 2016-07-15 08:40:46 UTC
nadith committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=054cf21948e86cbe5b809fa61c7a99d27fc8cf5d

tdf#100726: Improve readability of OUString concatenation in basic module

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 9 Commit Notification 2016-08-01 06:15:44 UTC
nadith committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=efef273e2c61b19a63572a71b103e3b1490f15af

tdf#100726: Improve readability of OUString concatenation

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 10 Commit Notification 2016-08-04 05:50:37 UTC
Rosen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=3c9d834af059bf3e9485ab9f0190733aa21dd9a6

tdf#100726 Improve readability of OUString concatanations

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 11 Commit Notification 2016-08-04 05:54:20 UTC
nadith committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c85a3ac70d813eef9baa9a5592c0a2d724bb9038

tdf#100726: Improve readability of OUString concatenation

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 12 Commit Notification 2016-08-04 09:34:57 UTC
nadith committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=bd4c80b856a145563ba38242705e87028b1e0ed6

tdf#100726: Improve readability of OUString concatenation

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 13 Commit Notification 2016-08-24 06:56:51 UTC
Gökhan Gurbetoğlu committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c899cc46fc6522b7fd7c243bf7a67dfbe2e1e586

tdf#100726 - Improve readability of OUString concatanations

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 14 Commit Notification 2016-08-25 06:56:23 UTC
Gökhan Gurbetoğlu committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=5d5f1de548c1086dd3476e40bfe06aa4a5830a6d

tdf#100726 - Improve readability of OUString concatanations

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 15 Commit Notification 2016-08-25 14:55:21 UTC
Gökhan Gurbetoğlu committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b7bf1ba2136b3d1e031673e7b541c6181e95ff61

tdf#100726 - Improve readability of OUString concatanations

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 16 Commit Notification 2016-08-26 07:16:53 UTC
Gökhan Gurbetoğlu committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=674e0f0b43392a7e7fa515dad8427ccc901f7a01

tdf#100726 - Improve readability of OUString concatanations

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 17 Commit Notification 2016-09-23 07:03:02 UTC
Asela Dasanayaka committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=bb06f2c00eea15c287fc06988d6f9b1f21098936

tdf#100726 Improve readability of OUString concatination

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 18 Commit Notification 2016-10-02 10:40:18 UTC
e12346 committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=793dae3e90692467c0f111cf5b127209f7f522fa

tdf#100726 Improve readability of OUString concatination

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 19 Commit Notification 2016-10-03 06:35:41 UTC
Chamal committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=bc4715ddac99ca104e1ba6b313f089cfe4039381

tdf#100726 Improve readability of OUString concatination

It will be available in 5.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 20 Tor Lillqvist 2016-10-03 07:14:23 UTC
I think we need to re-evaluate whether this is a good idea or not. To me, the type of changes that this bug encourages people to do, is just change for change's sake. It is very much a matter of taste which you find "more readable". (Example: bc4715ddac99ca104e1ba6b313f089cfe4039381 ) .

Surely we are not running out of much more obvious cleanups that we have a consensus they would make the code more readable?
Comment 21 Tor Lillqvist 2016-10-03 07:16:35 UTC
On the other hand, for instance 793dae3e90692467c0f111cf5b127209f7f522fa is IMHO an improvement, as one of the string literals was already split into several lines.
Comment 22 jani 2016-10-03 07:19:52 UTC
(In reply to Tor Lillqvist from comment #21)
> On the other hand, for instance 793dae3e90692467c0f111cf5b127209f7f522fa is
> IMHO an improvement, as one of the string literals was already split into
> several lines.

I agree with you, changes just for the sake of changes is not even a real easyHack
(just reviewed 5 patches correcting typos in comments).

I will bring it up at the next ESC, and see if we can have consensus to close bugs like this, to prevent too much noise in the git log.
Comment 23 Commit Notification 2017-02-15 14:48:23 UTC
Fakabbir Amin committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=445a3d0a8d891b872b7fb0611e98de508d4fa1ae

tdf#100726 Improved readability in sc directory

It will be available in 5.4.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 24 Commit Notification 2017-02-16 08:51:57 UTC
Fakabbir Amin committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9d8c206ee4a5c130e11a4e786b4286f3362f9ca1

tdf#100726 Improved readability of OUString concatenations

It will be available in 5.4.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 25 Udaree Kanewala 2017-09-26 18:06:56 UTC
Hi,

I would like to contribute to this bug. Is this still open?
Comment 26 Muhammet Kara 2017-09-27 07:08:25 UTC
(In reply to Udaree Kanewala from comment #25)
> Hi,
> 
> I would like to contribute to this bug. Is this still open?

Hey! Glad to hear that you are interested!

Since the status of the bug is "NEW", it seems open. Feel free to send a patch on gerrit. No need to assign this bug to yourself.
Comment 27 Commit Notification 2017-10-13 05:40:29 UTC
Tjipke van der Heide committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7ada9875ac6d340918eda85bcdba17bd19a20454

tdf#100726 improved readability of OUString/Ostring concatenation

It will be available in 6.0.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 28 Commit Notification 2018-04-17 06:17:19 UTC
Gökhan Gurbetoğlu committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=37729618021bb44b9c6ba37dc72a1d3111ce0ffb

tdf#100726 - Improve readability of OUString concatanations

It will be available in 6.1.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 29 Commit Notification 2018-07-09 06:12:28 UTC
Ulkem Kasapoglu committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=79c0328f4824300426333ba05c43bb300e2de4e7

tdf#100726: Improve readability of OUString/OString concatenations

It will be available in 6.2.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 30 Muhammet Kara 2018-07-09 06:13:29 UTC
I think this easyhack can be considered complete.