Bug 89267 - Reduce the amount of copy-paste in sc/qa/unit/opencl-test.cxx.
Summary: Reduce the amount of copy-paste in sc/qa/unit/opencl-test.cxx.
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.2.0.0.alpha0+ Master
Hardware: Other All
: medium normal
Assignee: Pranav Kant
URL:
Whiteboard: ToBeReviewed
Keywords: difficultyBeginner, easyHack, skillCpp
Depends on:
Blocks:
 
Reported: 2015-02-09 17:12 UTC by Tor Lillqvist
Modified: 2017-02-14 08:57 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
WIP: Only Sample implementation (2.71 KB, patch)
2015-02-20 17:24 UTC, Pranav Kant
Details
WIP: Only Sample implementation (2.88 KB, patch)
2015-02-20 21:09 UTC, Pranav Kant
Details
WIP: Only Sample implementation (2.97 KB, patch)
2015-02-21 03:39 UTC, Pranav Kant
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Lillqvist 2015-02-09 17:12:23 UTC
There are tons of functions in sc/qa/unit/opencl-test.cxx that are basically copies of the same template with a few details changed. Just look at testMathFormulaRoundUp(), testMathFormulaRoundDown(), testMathFormulaInt() for instance. This should be re-factored in the obvious fashion.
Comment 1 Tor Lillqvist 2015-02-09 17:13:24 UTC
(And I don't mean by using C++ templates.)
Comment 2 Tor Lillqvist 2015-02-09 17:14:02 UTC
(But if you can be convincing enough, sure, that would work, too. It's a unit test so code bloat is not an issue.)
Comment 3 Tor Lillqvist 2015-02-09 17:22:18 UTC
I guess this is a duplicate of bug #39593, but maybe good to have a very specific case of copy-paste as a separate bug, too.
Comment 4 Pranav Kant 2015-02-20 16:05:30 UTC
I would like to take this up and resolve this issue as my first contribution to libreoffice.
Comment 5 Tor Lillqvist 2015-02-20 16:09:15 UTC
Good, please go ahead. First learn to build LibreOffice for your platform!

(It is not necessary to assign the task to yourself, as long as you mention that you are working on it in a comment.)
Comment 6 Pranav Kant 2015-02-20 17:24:47 UTC
Created attachment 113548 [details]
WIP: Only Sample implementation

This is just a sample implementation. I just thought it would be better to have your input on this before doing it for the whole file.
Comment 7 Pranav Kant 2015-02-20 20:06:30 UTC
I can observe the repetitive code and it shouldn't be hard to create a wrapper function over it as in above attached patch.

However, there are few functions that make use of ScDocShell::DoHardRecalc() instead of ScDocument::CalcAll(). I was wondering what is the difference between a ScDocShell::DoHardRecalc() and ScDocument::CalcAll(). I might be able to wrap these 4-5 functions that make use of DoHardRecalc too and reduce more amount of code. Otherwise, we can also choose to not wrap these functions.
Comment 8 Pranav Kant 2015-02-20 21:09:36 UTC
Created attachment 113558 [details]
WIP: Only Sample implementation
Comment 9 Pranav Kant 2015-02-21 03:39:03 UTC
Created attachment 113568 [details]
WIP: Only Sample implementation
Comment 10 Commit Notification 2015-03-06 21:08:27 UTC
Pranav Kant committed a patch related to this issue.
It has been pushed to "master":

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

tdf#89267: Reduce duplicate code

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 mridul 2015-03-07 19:19:02 UTC
i am sorry but i am unable to understand what to do after i build the libre office.i mean where can i find these files after building the office?please help me
Comment 12 Brent Ritzema 2015-04-07 00:43:42 UTC
Is there still work to be done here? Or has the previous commit already covered everything wanted?
Comment 13 Tor Lillqvist 2015-04-07 06:36:19 UTC
How about taking a look in the source file in question and check if you see any?
Comment 14 Robinson Tryon (qubit) 2015-12-14 06:32:10 UTC Comment hidden (obsolete)
Comment 15 Robinson Tryon (qubit) 2016-02-18 14:51:45 UTC Comment hidden (obsolete)
Comment 16 Tor Lillqvist 2016-03-29 10:15:18 UTC
I think this can be marked as RESOLVED:FIXED, yeah.
Comment 17 Tor Lillqvist 2016-03-29 10:19:04 UTC
Actually it turned out that this refactoring caused a memory usage issue. The ScDocShellRef variables were factored out to be members in the ScOpenCLTest class instead. The ScOpenCLTest objects for all the test functions are created in advance, then the functions are run, and only then the ScOpenCLTest objects are destroyed. This meant that the data held on to by the ScDocShellRef fields was not released, but accumulated as each test function loaded a test document (twice!). But I have a fix for that which I will push in a moment.
Comment 18 tommy27 2016-04-03 06:05:35 UTC
@Tor
is your fix already merged?
Comment 19 Tor Lillqvist 2016-04-03 06:58:38 UTC
Yes.