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 .
Assigning to myself
OK, if you promise to assign back to the default if you don't do this within, say, two weeks...
(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 :)
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?
(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.
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?
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 :-)
Thanks for the help. Here is the patch to get rid of the ScaList. https://gerrit.libreoffice.org/#/c/15177/
Hey I'm new at this. Where can get started?
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...
(Which you didn't, but just in case;)
@tml: Nope, it hasn't. The submitted patch has been rejected.
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
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.
Another small one: https://gerrit.libreoffice.org/17573
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.
Another: https://gerrit.libreoffice.org/#/c/17579/
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.
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.
I'm assigning this to myself to prevent others from having merge conflicts. I still welcome the help regardless, it just makes it clearer.
New Changes: https://gerrit.libreoffice.org/17651 tdf#90222: Removed ScaFuncDataList type https://gerrit.libreoffice.org/17652 tdf#90222: Removed ScaList from pricing scaddin
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.
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.
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
And on a different branch (isolated code changes) New Changes: https://gerrit.libreoffice.org/17680 Removed another FuncDataList collection
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.
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?
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.
Good point. I'll try to get that change in soonish.
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>
Migrating Whiteboard tags to Keywords: (easyHack, difficultyBeginner, skillCpp, topicCleanup) [NinjaEdit]
Added back target:5.1.0
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit]
I just grep'd the source tree and this seems to be resolved.