Bug 90222 - replace ScaList in scaddins with a std container
Summary: replace ScaList in scaddins with a std container
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium enhancement
Assignee: Ian Gilham
URL:
Whiteboard: target:5.1.0 ToBeReviewed
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2015-03-25 08:16 UTC by David Tardon
Modified: 2017-02-14 08:57 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 David Tardon 2015-03-25 08:16:31 UTC
The scaddins module contains a home-grown container class named ScaList. It is a generic base class, from which are derived other classes holding concrete data types. The code is even duplicated in two places. It should all be replaced by a std container type, most likely std::vector.

The definition of the classes is in scaddins/source/pricing/pricing.?xx and scaddins/source/datefunc/datefunc.?xx .
Comment 1 Pieter Adriaensen 2015-03-25 15:44:49 UTC
Assigning to myself
Comment 2 Tor Lillqvist 2015-03-25 16:06:08 UTC
OK, if you promise to assign back to the default if you don't do this within, say, two weeks...
Comment 3 Pieter Adriaensen 2015-03-25 21:31:10 UTC
(In reply to Tor Lillqvist from comment #2)
> OK, if you promise to assign back to the default if you don't do this
> within, say, two weeks...

Ok, but I'm planning on fixing it though :)
Comment 4 Pieter Adriaensen 2015-03-26 09:45:31 UTC
I pushed a patch to gerrit

https://gerrit.libreoffice.org/#/c/15012/
tdf#90222: replace ScaList in scaddins with a std container

I still have a question though: The ScaList is always used to put data of the same class in. So isn't it better to change it to vector<T*> instead of being a vector<void*> and doing static_casts?
Comment 5 David Tardon 2015-03-26 12:05:31 UTC
(In reply to Pieter Adriaensen from comment #4)
> I still have a question though: The ScaList is always used to put data of
> the same class in. So isn't it better to change it to vector<T*> instead of
> being a vector<void*> and doing static_casts?

Of course it is better! I assumed this is something that does not have to be explicitly mentioned... Also, it should be vector<T>, if possible.
Comment 6 Pieter Adriaensen 2015-04-06 18:05:57 UTC
I fixed the issue and I'm now fixing the code replication of pricing and datefunc by making a base class that they will inherit from. But I can't find where to put the new class in the makefiles. Any suggestions?
Comment 7 Michael Meeks 2015-04-06 20:05:56 UTC
Hi Pieter - there is no need to add a new class to the makefiles, but assuming you mean a new source module try:

scaddins/Library_*.mk eg.

$(eval $(call gb_Library_add_exception_objects,analysis,\
	scaddins/source/analysis/analysis \
	scaddins/source/analysis/analysishelper \

These guys are all paths to .cxx files eg. just add your path to the relevant libraries' Library_*.mk and you should be fine.

Looking forward to reading the patch. If you do two un-related changes: eg. fixing the STL container issue, and then re-factoring copy/paste code - please submit these as separate commits to make the history comprehensible !

Good stuff :-)
Comment 8 Pieter Adriaensen 2015-04-06 21:36:07 UTC
Thanks for the help.

Here is the patch to get rid of the ScaList.
https://gerrit.libreoffice.org/#/c/15177/
Comment 9 ashutosh75 2015-07-07 02:39:17 UTC
Hey I'm new at this.
Where can get started?
Comment 10 Tor Lillqvist 2015-07-07 05:44:53 UTC
Start by readin the previous comments? Based on them, it seems to me that this has already been done? Don't start by assigning it to yourself before you have any idea what to do...
Comment 11 Tor Lillqvist 2015-07-07 05:45:32 UTC
(Which you didn't, but just in case;)
Comment 12 David Tardon 2015-07-07 17:07:02 UTC
@tml: Nope, it hasn't. The submitted patch has been rejected.
Comment 13 Ian Gilham 2015-08-06 15:39:02 UTC
Hi there. It's my first time so please be gentle.

First, a caveat: my antivirus keeps complaining about a trojan while trying to run CppunitTest_filter_tiff_test. The test currently fails on master, but it is not clear if that is because the AV kills it or there are genuine failures. I'm waiting for Jenkins for automated test results.

How does this look for a first step?

https://gerrit.libreoffice.org/17543
Comment 14 Commit Notification 2015-08-07 07:04:54 UTC
Ian committed a patch related to this issue.
It has been pushed to "master":

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

tdf#90222: Removed ScaStringList and replaced all uses with std::vector

It will be available in 5.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 15 Ian Gilham 2015-08-07 10:05:30 UTC
Another small one: https://gerrit.libreoffice.org/17573
Comment 16 Commit Notification 2015-08-07 14:03:06 UTC
Ian committed a patch related to this issue.
It has been pushed to "master":

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

tdf#90222: Removed redundant collection type

It will be available in 5.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 17 Ian Gilham 2015-08-07 16:11:44 UTC
Another: https://gerrit.libreoffice.org/#/c/17579/
Comment 18 tux3 2015-08-07 22:42:49 UTC
I started working on this bug yesterday too, oops!

I was about to send a patch that removes ScaList from datefunc, but I see I'm a bit late to take on this bug.

So here it is anyways, rebased it on top of Ian's changes:
https://gerrit.libreoffice.org/17584

Now I'll go see if I can assign myself something else to work on.
Comment 19 Commit Notification 2015-08-11 07:11:05 UTC
Ian committed a patch related to this issue.
It has been pushed to "master":

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

tdf#90222: Removed ScaFuncDataList type

It will be available in 5.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 20 Ian Gilham 2015-08-11 09:04:39 UTC
I'm assigning this to myself to prevent others from having merge conflicts. I still welcome the help regardless, it just makes it clearer.
Comment 21 Ian Gilham 2015-08-11 15:00:49 UTC
New Changes:

https://gerrit.libreoffice.org/17651 tdf#90222: Removed ScaFuncDataList type
https://gerrit.libreoffice.org/17652 tdf#90222: Removed ScaList from pricing scaddin
Comment 22 Commit Notification 2015-08-12 11:00:32 UTC
Ian committed a patch related to this issue.
It has been pushed to "master":

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

tdf#90222: Removed ScaFuncDataList type

It will be available in 5.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 23 Commit Notification 2015-08-12 11:12:41 UTC
Ian committed a patch related to this issue.
It has been pushed to "master":

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

tdf#90222: Removed ScaList from pricing scaddin

It will be available in 5.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 24 Ian Gilham 2015-08-12 14:42:00 UTC
New Changes:

https://gerrit.libreoffice.org/17678 tdf#90222: Removed ScaList and cleaned up usages
https://gerrit.libreoffice.org/17679 migrated some raw pointers to unique_ptr's
Comment 25 Ian Gilham 2015-08-12 15:10:30 UTC
And on a different branch (isolated code changes)

New Changes:

https://gerrit.libreoffice.org/17680 Removed another FuncDataList collection
Comment 26 Commit Notification 2015-08-13 06:51:40 UTC
Ian committed a patch related to this issue.
It has been pushed to "master":

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

tdf#90222: Removed ScaList and cleaned up usages

It will be available in 5.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 27 Ian Gilham 2015-08-17 15:44:56 UTC
We might be hitting the point of diminished returns on this ticket now. Most of the offending code has been removed.

Is there any desire to keep going and continue making small removals in this area?
Comment 28 David Tardon 2015-08-20 14:42:33 UTC
Yes, the intial objective has been achieved. There is one small possible improvement yet, though: the remaining list classes typically hold a vector of pointers and the elements are dynamically allocated on insertion. These could be changed to contain the elements as values.
Comment 29 Ian Gilham 2015-08-21 16:33:18 UTC
Good point. I'll try to get that change in soonish.
Comment 30 Ian Gilham 2015-08-21 17:02:12 UTC
Okay, I've naïvely changed all the vector<Thing*> instances to vector<Thing>.

New Changes:
  https://gerrit.libreoffice.org/17914 Changed vector<Complex*> to vector<Complex>
  https://gerrit.libreoffice.org/17915 alter vector<ConvertData>* to vector<ConvertData>
Comment 31 Robinson Tryon (qubit) 2015-12-13 11:04:04 UTC Comment hidden (obsolete)
Comment 32 Chris Sherlock 2016-02-14 07:49:06 UTC
Added back target:5.1.0
Comment 33 Robinson Tryon (qubit) 2016-02-18 14:51:23 UTC Comment hidden (obsolete)
Comment 34 Ryan McCoskrie 2016-04-13 07:49:14 UTC
I just grep'd the source tree and this seems to be resolved.