Reading in: sal/osl/unx/signal.cxx and sal/osl/w32/signal.cxx there is significant copy/paste coding: /*****************************************************************************/ /* osl_addSignalHandler */ /*****************************************************************************/ oslSignalHandler SAL_CALL osl_addSignalHandler(oslSignalHandlerFunction Handler, void* pData) for example is ~the same; I imagine remove, raise etc. are much the same. Thanks !
Code should go into a new sal/osl/all/signal.cxx I guess =)
Migrating Whiteboard tags to Keywords: (easyHack, difficultyBeginner, skillCpp, topicCleanup)
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit]
I am working on it.
(In reply to Aleksas Pantechovskis from comment #4) > I am working on it. Please assign it to yourself, so we know it is not available to others. Have a look at https://wiki.documentfoundation.org/Development/GetInvolved rgds jan i.
Ok, somebody said to me few days ago that for EasyHacks it is better to just comment without assigning (not sure why) :)
(In reply to Aleksas Pantechovskis from comment #6) > Ok, somebody said to me few days ago that for EasyHacks it is better to just > comment without assigning (not sure why) :) I too cannot tell why, it might have been someone, who did not know that we have changed this quite a lot. Earlier nobody monitored easyHacks closely, now we track them every week. The reason for assigning, is primeraly, so that it get removed from the list in the Wiki. We have (sadly) had several easyhacks with multiple people working on it, and it is very sad to need to reject a good patch, just because someone else did the same patch a day earlier.
(In reply to Michael Meeks from comment #1) > Code should go into a new sal/osl/all/signal.cxx I guess =) Do we need to move only the duplicated functions to a new file or to delete these files in unx and win32 and move everything to all/signal.cxx using #ifdef's for platform specific stuff? For the first option, I thought there are maybe issues (at least for some platforms/...) if files have the same name. I asked about that on the IRC channel, tml_ too suspects that it is not a good idea and may cause issues on iOS, Android.
We should split the shared code out clean modules with no #ifdefs. Then we should sub-class them and/or re-use that in the Windows specific backends. Having a single file with lots of #ifdefs would be un-readable and unpleasant. But we are trying to get the smallest amount of most readable, clean code possible. If we can avoid ten lines of duplicate method code for a single line #if # else - then - perhaps that makes sense. Thanks !
(In reply to Michael Meeks from comment #9) > We should split the shared code out clean modules with no #ifdefs. > > Then we should sub-class them and/or re-use that in the Windows > specific backends. I am not sure if I fully understood what do you mean. You suggesting to refactor sal signal functions into classes? That is to put shared code into a base class and inherit classes for win and unix from it.
I'm sorry - I'm not entirely sure what is not clear about this. Some methods are very similar - and can be shared; I gave some examples of them: osl_addSignalHandler eg. Other methods are -highly- platform specific and should not be in the same module, but nicely split up per platform as now. There are a ton of different ways to achieve that; from just putting different methods in different files, and sharing headers - to having differently named internal platform specific helper functions, to some sub-classing approach. Pick one - and use it to make the code a) less duplicated and b) smaller & prettier =) it will be necessary to use some creative intelligence here - I'm certain that you can do it without further input; just by re-reading what is in this bug =) Thanks !
Yes, I understand that, I was just asking if there is an approach preferred for such situations and if there is something specific about sal that needs to be considered, because as I understand it is one of the "core" modules used in many parts of LibreOffice. :) Also with any of the approaches I will probably need to create a new file. What do I need to do to add a new file to the build/which makefile I need to modify? As far as I understand from git grep - it is sal/Library_sal.mk? Or is there anything else I need to do when adding a new file?
Please invest some time to read around the build system and answer your question yourself. Or use git log --numstat to find a similar patch that added a file =)
I submitted my attempt to refactor it to gerrit.:) (have not built on Windows yet, only Linux)
Aleksas Pantechovskis committed a patch related to this issue. It has been pushed to "master": http://cgit.freedesktop.org/libreoffice/core/commit/?id=97858ca008fd8cf405e52795c8db415ef6c5afb3 tdf#93548 Refactor sal signal to reduce code duplication 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.
Seems solved