Bug 93548 - sal signal duplicate code redux ...
Summary: sal signal duplicate code redux ...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: framework (show other bugs)
Version:
(earliest affected)
5.0.0.0.beta1
Hardware: Other All
: medium normal
Assignee: Aleksas Pantechovskis
URL:
Whiteboard: ToBeReviewed target:5.2.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2015-08-20 17:58 UTC by Michael Meeks
Modified: 2017-02-14 08:58 UTC (History)
3 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 Michael Meeks 2015-08-20 17:58:49 UTC
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 !
Comment 1 Michael Meeks 2015-08-20 17:59:51 UTC
Code should go into a new sal/osl/all/signal.cxx I guess =)
Comment 2 Robinson Tryon (qubit) 2015-12-10 11:40:57 UTC Comment hidden (obsolete)
Comment 3 Robinson Tryon (qubit) 2016-02-18 14:52:26 UTC Comment hidden (obsolete)
Comment 4 Aleksas Pantechovskis 2016-03-07 19:19:55 UTC
I am working on it.
Comment 5 jani 2016-03-07 19:47:25 UTC
(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.
Comment 6 Aleksas Pantechovskis 2016-03-08 06:30:29 UTC
Ok, somebody said to me few days ago that for EasyHacks it is better to just comment without assigning (not sure why) :)
Comment 7 jani 2016-03-08 06:39:22 UTC
(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.
Comment 8 Aleksas Pantechovskis 2016-03-08 08:27:56 UTC
(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.
Comment 9 Michael Meeks 2016-03-14 15:53:41 UTC
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 !
Comment 10 Aleksas Pantechovskis 2016-03-15 16:12:53 UTC
(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.
Comment 11 Michael Meeks 2016-03-16 13:58:14 UTC
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 !
Comment 12 Aleksas Pantechovskis 2016-03-16 14:52:35 UTC
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?
Comment 13 Michael Meeks 2016-03-16 15:22:22 UTC
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 =)
Comment 14 Aleksas Pantechovskis 2016-03-23 17:06:47 UTC
I submitted my attempt to refactor it to gerrit.:) (have not built on Windows yet, only Linux)
Comment 15 Commit Notification 2016-03-29 15:58:06 UTC
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.
Comment 16 jani 2016-04-29 05:40:01 UTC
Seems solved