Bug Hunting Session
Bug 89756 - change from postfix operator++/-- to prefix operator++/--
Summary: change from postfix operator++/-- to prefix operator++/--
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:4.5.0 target:5.2.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks: 90564
  Show dependency treegraph
 
Reported: 2015-03-01 16:31 UTC by Markus Mohrhard
Modified: 2016-10-25 19:08 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
Patch for Bug ver1 (107.60 KB, patch)
2015-03-02 18:40 UTC, ababaaa
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Mohrhard 2015-03-01 16:31:00 UTC
The following file contains a list of places where we might want to switch from postfix to prefix operator++ and operator--. Each location needs to be checked and changed if it is really an issue:

http://www.viva64.com/external-pictures/txt/LibreOffice-V803.txt
Comment 1 ababaaa 2015-03-01 18:17:58 UTC
I want to work on this issue.
But what advantage will we get by changing postfix to prefix.
Can you Please explain that as it is my 1st bug.

Thanks for your help.
Comment 2 Markus Mohrhard 2015-03-01 18:22:00 UTC
the prefix version is faster as it does not create a temporary object.

See e.g. http://stackoverflow.com/questions/24901/is-there-a-performance-difference-between-i-and-i-in-c and for iterators you are in the non-native type case.
Comment 3 ababaaa 2015-03-01 18:24:24 UTC
I searched google and I find that prefix gives slightly better performance than the postfix.So,this issue is workable.
I will soon commit a patch.
Thanks
Comment 4 Julien Nabet 2015-03-01 19:52:13 UTC
Following Lovekesh's comment, let's put this one to ASSIGNED
Comment 5 ababaaa 2015-03-02 18:40:29 UTC
Created attachment 113834 [details]
Patch for Bug ver1

Here is my first patch for LibreOffice Project.
Bugfix for Bug # 89756.
Comment 6 Julien Nabet 2015-03-02 21:22:10 UTC
Lovekesh: it could be useful you submit your patch on gerrit, see https://wiki.documentfoundation.org/Development/GetInvolved#Preparing_patches
Comment 7 Gülşah Köse 2015-03-17 15:55:15 UTC
Since the last information message has written two weeks ago, I assign this error myself and I working on it.
Comment 8 Gülşah Köse 2015-03-17 23:26:22 UTC
Hi Markus

I want to someting about commit style. I have to change code in many different file. Should i do micro commit for every file or a one commit includes all of the changes?
Comment 9 Gülşah Köse 2015-03-19 12:13:17 UTC
I sent following patch for this bug:

https://gerrit.libreoffice.org/#/c/14903/
Comment 10 Commit Notification 2015-03-20 09:51:09 UTC
Gulsah Kose committed a patch related to this issue.
It has been pushed to "master":

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

tdf#89756 Switched postfix to prefix operator++/--

It will be available in 4.5.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 Fernando Montes 2015-03-22 22:25:04 UTC
does this still need work on it? i dont really understand how the pick a easy hack works
Comment 12 Ahmad 2015-03-24 15:01:56 UTC
As Gulsah Kose didn't fix all the mentioned files, I fixed the rest and uploaded my patch here https://gerrit.libreoffice.org/#/c/14975/4
Comment 13 Björn Michaelsen 2015-07-06 16:55:39 UTC
Comment on attachment 113834 [details]
Patch for Bug ver1

The attached patch is huge and certainly doesnt apply any more cleanly. Please use gerrit in the future as suggested in comment 6. As such, marking the patch as obsolete as it cant be merged as-is.
Comment 14 Robinson Tryon (qubit) 2015-12-14 06:35:14 UTC Comment hidden (obsolete)
Comment 15 langhoffwilliam 2016-01-30 02:32:37 UTC
Is this still being worked on? I'm new to development and I'd like to work on it if thats okay.
Comment 16 jani 2016-01-30 10:04:31 UTC
Resetting owner, as works seems to be stopped. Gerrit patch is abandoned.
Comment 17 Julien Nabet 2016-01-30 10:23:47 UTC
(In reply to langhoffwilliam from comment #15)
> Is this still being worked on? I'm new to development and I'd like to work
> on it if thats okay.

No problem, you can work on it! :-) Here's the start page to contribute about dev for LO beginners:
https://wiki.documentfoundation.org/Development
Comment 18 Commit Notification 2016-01-31 13:39:14 UTC
erdemdemirkapi committed a patch related to this issue.
It has been pushed to "master":

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

tdf#89756 swich postfix to prefix operators

It will be available in 5.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 19 Commit Notification 2016-01-31 17:10:24 UTC
jan iversen committed a patch related to this issue.
It has been pushed to "master":

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

Revert "tdf#89756 swich postfix to prefix operators"

It will be available in 5.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 20 Tor Lillqvist 2016-02-01 05:27:59 UTC
Have we already fixed the occurrences mentioned in the original comment? Are there only simple integer or pointer ++/-- operators left? In that case I suggest we resolve this bug as fixed. Otherwise we will just get suggestions that mechanically blindly change all post-increments to pre-increments without taking the difference in semantics into account, and thus introducing bugs in the cases where it does make a difference.
Comment 21 jani 2016-02-01 16:47:50 UTC
following suggestion from Tor followed by direct discussions.

I will cleanup the gerrit patches (which are faulty)
Comment 22 Robinson Tryon (qubit) 2016-02-18 16:37:12 UTC Comment hidden (obsolete)