Bug Hunting Session
Bug 63154 - replace tools/solar.h macros with osl versions
Summary: replace tools/solar.h macros with osl versions
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
4.0.1.1 rc
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:4.1.0 target:4.3.0 target:6.1.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2013-04-05 09:54 UTC by Michael Meeks
Modified: 2017-12-13 21:20 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
Bug 63154 - replace tools/solar.h macros with osl versions (2.51 MB, patch)
2013-05-03 16:26 UTC, himanshu
Details
cleanup patch (1.47 KB, patch)
2013-05-15 14:19 UTC, Michael Meeks
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2013-04-05 09:54:25 UTC
Some of our oldest code uses the tools/inc/tools/solar.h header to do various things. Things like:

#define _LF     ((char)0x0A)
#define _CR     ((char)0x0D)

Are poorly namespaced, and presumably under-used too, as well as being duplicated everywhere:

basctl/source/inc/bastypes.hxx:#define LINE_SEP_CR     0x0D

eg. presumably it'd be good to have these defined in sal in some sensibly namespaced way if they are not already, and uses those throught the code.

Then we have these guys:

#ifndef __cplusplus
#ifndef min
#define min(a,b)    (((a) < (b)) ? (a) : (b))
#endif
#ifndef max
#define max(a,b)    (((a) > (b)) ? (a) : (b))
#endif
#endif

#ifdef __cplusplus
template<typename T> inline T Min(T a, T b) { return (a<b?a:b); }
template<typename T> inline T Max(T a, T b) { return (a>b?a:b); }
template<typename T> inline T Abs(T a) { return (a>=0?a:-a); }
#endif

Which look (to me) as if they should be re-written to use std::min std::max etc. (I suspect) and then removed.

These guys:

tools/inc/tools/solar.h:#define EXTERN_C    extern "C"
tools/inc/tools/solar.h:#define EXTERN_C

should be in-lined at the tiny number of sites that macro is used and it should be removed.

The SVBT16ToShort macros look like they could use each call site tweaking to use equivalent sal macros [ are they equivalent to OSL_NETWORD* ? ].

Anyhow - lots of good cleanup possible there.

Thanks ! :-)
Comment 1 Michael Meeks 2013-04-05 09:55:09 UTC
Stephan might have some thoughts on this too :-)
Comment 2 Marcos Souza 2013-04-11 17:06:20 UTC
Guys, and about this:

#define STRING_CONCAT3( s1, s2, s3 ) \
    s1 s2 s3

// dll file extensions

#if defined WNT
#define SVLIBRARY( Base ) \
    STRING_CONCAT3( Base, "lo", ".dll" )
#elif defined MACOSX
#define SVLIBRARY( Base ) \
    STRING_CONCAT3( "lib", Base, "lo.dylib" )
#elif defined UNX
#define SVLIBRARY( Base ) \
    STRING_CONCAT3( "lib", Base, "lo.so" )
#else
  #error unknown platform
#endif

Where we can put this?

This is the file list of who uses the SVLIBRARY macro:
marcos@jedi:~/gitroot/core$ git grep SVLIBRARY
connectivity/source/drivers/mozab/MDriver.cxx:                "$libname$", OUString( SVLIBRARY( "mozabdrv" ) )
connectivity/source/drivers/mozab/MDriver.cxx:    const OUString sModuleName(SVLIBRARY( "mozabdrv" ));
connectivity/source/drivers/mozab/MServices.cxx:        const OUString sModuleName(SVLIBRARY( "mozabdrv" ));
connectivity/workben/testmoz/main.cxx:    , SVLIBRARY( "dtransX11" )        // OBR
sc/source/ui/attrdlg/scabstdlg.cxx:    aStrBuf.appendAscii( SVLIBRARY("scui") );
sc/source/ui/docshell/impex.cxx:    OUString sFilterLib(SVLIBRARY("scfilt"));
sd/source/filter/sdfilter.cxx:    String aTemp(OUString(SVLIBRARY("?")));
sfx2/source/appl/app.cxx:    static OUString aLibName( SVLIBRARY( "basctl"  ) );
sfx2/source/appl/app.cxx:    static OUString aLibName( SVLIBRARY( "basctl"  ) );
sfx2/source/appl/appinit.cxx:        static OUString aLibName( SVLIBRARY( "cui" ) );
sfx2/source/appl/appserv.cxx:    static OUString aLibName( SVLIBRARY( "basctl"  ) );
sfx2/source/appl/appserv.cxx:    static OUString aLibName( SVLIBRARY( "basctl"  ) );
svtools/source/misc/svtaccessiblefactory.cxx:                const OUString sModuleName( SVLIBRARY( "acc" ));
svx/source/form/dbtoolsclient.cxx:            const OUString sModuleName( SVLIBRARY( "dbtools" )
sw/source/filter/basflt/fltini.cxx:        bool ok = msword_.loadRelative( &thisModule, SVLIBRARY( "msword" ), SAL_LOADMODULE_GLOBAL | SAL_LOAD
sw/source/ui/dbui/swdbtoolsclient.cxx:        const OUString sModuleName(RTL_CONSTASCII_USTRINGPARAM(SVLIBRARY("dbtools")));
toolkit/source/helper/accessibilityclient.cxx:                const OUString sModuleName( SVLIBRARY( "acc" ) );
tools/inc/tools/solar.h:#define SVLIBRARY( Base ) \
tools/inc/tools/solar.h:#define SVLIBRARY( Base ) \
tools/inc/tools/solar.h:#define SVLIBRARY( Base ) \
vcl/source/filter/FilterConfigCache.cxx:        OUString sTemp(SVLIBRARY("?"));

Maybe on sal or osl...?
Comment 3 Stephan Bergmann 2013-04-12 06:58:33 UTC
SVLIBRARY should not go into a URE header, as it codifies non-URE knowledge---namely, that some LO libraries carry a "lo" in their name.

The best solution appears to be to keep this in a non-URE header.  If there's no better place than tools, maybe create a dedicated tools/inc/tools/lomodulename.hxx with a TOOLS_LOMODULENAME macro.  And in any event, let that macro re-use SAL_MODULENAME (sal/inc/osl/module.h), TOOLS_LOMODULENAME(name) SAL_MODULENAME(name "lo").

The second best solution would be to change the existing uses of SVLIBRARY directly to SAL_MODULENAME, hard-coding the "lo" into them.
Comment 4 Marcos Souza 2013-04-16 13:26:21 UTC
Thanks for the tips Stephan!

I look at this when my patch about std::max get checked by the build checks. Maybe Windows build could broke with this commit.

That patch is big, so I want to get it merged before touch this macro :)
Comment 5 Commit Notification 2013-04-20 11:13:03 UTC
Marcos Paulo de Souza committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154: Change Min/Max/Abs for std::min/max/abs



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 6 Michael Stahl (CIB) 2013-04-23 11:41:43 UTC
not sure who had the crazy idea of hard-coding "lo" into dozens of library loading calls... but clearly that silly "lo" suffix serves no useful purpose and we should just get rid of it in the build system, in which case SVLIBRARY can be trivially replaced with SAL_MODULENAME.
Comment 7 Björn Michaelsen 2013-04-23 12:44:54 UTC
@mst: Thinking about it, I believe now it was rejected so that there is some more visibility which libs are UNO API stable and which ones are libreoffice-internal-you-can-go-wild-on-these ...
Comment 8 Michael Meeks 2013-04-23 12:58:42 UTC
> @mst: Thinking about it, I believe now it was rejected so that there is some
> more visibility which libs are UNO API stable and which ones are
> libreoffice-internal-you-can-go-wild-on-these ...

FWIW, another project has dropped the bogus suffix here and cleaned this code up :-) IMHO we should do the same - it should simplify scp2/ etc. and remove the grim macro-ness here.

The ABI stable URE libraries are currently in ure/lib too which is quite a separation.
Comment 9 Marcos Souza 2013-04-23 13:04:30 UTC
So, can I do as M Stahl said? Remove the SUFFIX variable and all usages and use only SAL_MODULENAME?
Comment 10 Michael Meeks 2013-04-25 14:59:40 UTC
Marcos - so sorry for this; it seems the SVLIBRARY one is best not to touch; but the other macros in that header should be ok to work out of the code (seems like you made a great start there with min/max etc.)
Comment 11 Commit Notification 2013-04-26 11:58:17 UTC
Marcos Paulo de Souza committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154: Remove all usages of the macro EXTERN_C



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 12 Marcos Souza 2013-04-26 21:52:04 UTC
Guys, about this:
#define _CR     ((char)0x0D)

After a git grep:
marcos@jedi:~/gitroot/core$ git grep "0x0D" | grep -w "0x0D" | less | grep "define"
basctl/source/inc/bastypes.hxx:#define LINE_SEP_CR     0x0D
bluez_bluetooth/inc/bluetooth/hci.h:#define EVT_QOS_SETUP_COMPLETE 0x0D
connectivity/source/drivers/dbase/DTable.cxx:#define FIELD_DESCRIPTOR_TERMINATOR 0x0D
filter/source/graphicfilter/ios2met/ios2met.cxx:#define GOrdSBgMix 0x0D   /* 0 1 Set background mix          */
include/tools/solar.h:#define _CR     ((char)0x0D)
rsc/source/rscpp/cpp.h:#define CR              0x0D            /* carriage return              */
rsc/source/rscpp/cpp.h:#define CR              0x0D            /* carriage return              */

Where can I put this define:
#define _CR     ((char)0x0D)

This bug entry says in somewhere within sal, but where...?

Thanks!
Comment 13 Michael Meeks 2013-04-27 07:09:01 UTC
I'd be inclined to put it into the include/rtl/string.h file and call them:

RTL_SEP_CR
RTL_SEP_LF

and then replace all the instances you found of both with these - cleaning out all the duplicate defines etc. :-)

Of course, we can all discuss for ages where to put them and what to call them - but hopefully that'll do for now :-)

Many thanks !
Comment 14 Stephan Bergmann 2013-04-29 07:35:41 UTC
(In reply to comment #13)
> RTL_SEP_CR
> RTL_SEP_LF

RTL_CHAR_CR is probably clearer.  And define it directly to be a char, '\x0D'.

(This IMO shows that having defines for these characters is likely a poor idea to begin with, as they cannot be composed into string literals, force a specific type that might not be appropriate in every usage scenario, and besides---what is the difference between writing 'A' and '\x0D'?  Or is the world really illiterate wrt ASCII folklore?)
Comment 15 Michael Meeks 2013-04-29 08:18:05 UTC
Marco - FYI Stephan is the sal/ expert - so I'd follow his prettier names :-) it'd be great to get all of that cleaned up. Thanks !
Comment 16 Marcos Souza 2013-04-30 12:47:20 UTC
I will work in this paralleled with autoinstallation :)

Maybe in two days I will send a patch about the _CL macros, following what Michael/Stephan said!
Comment 17 Don't use this account, use tml@iki.fi 2013-05-02 11:43:44 UTC
Personally I would just use '\r' instead of FOO_BAR_CR (expanding to '0x0D') and '\n' instead of FOO_BAR_LF (expanding to '\0x0A'). People unfamiliar with the code, seeing FOO_BAR_CR or LF think it is something stranger than what it actually is and need to look it up anyway.

On all current and future platforms where LibreOffice runs they mean the same as far as I know. Avoiding \n and \r is just folklore and cargo cult, perhaps caused by fearing "they mean different things on Windows". No, they don't. Text vs. binary file IO and the mapping of \n in that is a completely different thing.

Of course, as always, I am prepared to change my opinion if proven wrong.
Comment 18 Eike Rathke 2013-05-03 16:16:33 UTC
IIRC the macros coded as hex constants were only introduced because the old Mac (not MacOSX or anything that new ;-) compiler did not accept the escaped form. Nowadays '\r' and '\n' should be no problem..
Comment 19 Marcos Souza 2013-05-03 16:23:48 UTC
himanshu150891,

I'm working in this bug...

maybe you want to help with something?

I'm replacing the _CR macros...
Comment 20 himanshu 2013-05-03 16:26:54 UTC
Created attachment 78815 [details]
Bug 63154 - replace tools/solar.h macros with osl versions

duplicates removed 
and renamed as CR & LF and defined in rtl/string.h
Comment 21 Don't use this account, use tml@iki.fi 2013-05-03 16:49:58 UTC
himanshu, please, we don't need two people working on the same thing. (And this is a too trivial task to count as experience for GSoC anyway.)
Comment 22 Commit Notification 2013-05-09 09:58:28 UTC
Marcos Paulo de Souza committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154: Remove _CL and _LF from solar.h



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 Marcos Souza 2013-05-15 10:52:34 UTC
So, a lot of junk was removed :)

Now, there is this SVBT16ToShort...

You guys said about the OSL_NETWORD... but where I can find this?

And how to replace...?

Thanks since now!
Comment 24 Michael Meeks 2013-05-15 14:19:13 UTC
Created attachment 79349 [details]
cleanup patch

I'd love to have the attached cleanup checked & pushed to master after the -4-1 branch-off [ it's too expensive to force a re-build for everyone now just before the 4.1 freeze IMHO ].
Comment 25 Stephan Bergmann 2013-05-16 08:28:07 UTC
(In reply to comment #24)
> I'd love to have the attached cleanup checked & pushed to master after the
> -4-1 branch-off [ it's too expensive to force a re-build for everyone now
> just before the 4.1 freeze IMHO ].

Get rid of the STRING_CONCAT2 cargo cult.
Comment 26 Michael Meeks 2013-05-16 11:11:13 UTC
> Get rid of the STRING_CONCAT2 cargo cult.

Hah ;-) I was wondering what that was good for; presumably there was actually a purpose of this sort of thing in the deep past - but yes; nonsensical.
Comment 27 Gergely Mate 2013-07-19 13:33:51 UTC
Am I wrong when I think that this bug is resolved? What's remaining?
Comment 28 Michael Meeks 2013-07-19 14:04:58 UTC
> Now, there is this SVBT16ToShort...
>
> You guys said about the OSL_NETWORD... but where I can find this?

Sure - so:

$ git grep OSL_NET

And I guess read both sides to check how they function before starting to replace :-)

Might be nice to rid ourselves of:

#define UniString       ... and the XubString and so on - but then that would be a large mechanical change to bloat the repo, perhaps better to wait until all UniString usage is killed.

So yes - this job is nearly done :-)
Comment 29 Björn Michaelsen 2013-10-04 18:46:16 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 30 Robinson Tryon (qubit) 2013-10-19 00:28:08 UTC Comment hidden (obsolete)
Comment 31 Marcos Souza 2013-11-02 06:48:23 UTC
Can I removed finally the DELETEZ macro too?
Comment 32 Michael Meeks 2013-11-02 19:49:11 UTC
There are 600 or so hits of DELETEZ, in each case we'd need to work out whether the memory could be re-used inadvertently and I guess add a manual NULL as/when we think it could be; that my be somewhat hard to verify of course.

I'm not against that, clearly any usage of DELETEZ of a member in a destructor is bogus eg.

accessibility/source/extended/accessibletabbarbase.cxx-

AccessibleTabBarBase::~AccessibleTabBarBase()
{
    ClearTabBarPointer();
    DELETEZ( m_pExternalLock );
}

or

cui/source/tabpages/macroass.cxx

_SfxMacroTabPage::~_SfxMacroTabPage()
{
    DELETEZ( mpImpl );
}

And those can be removed safely. I guess we should consult some C++ guru - who (no doubt) would recommend one of the bewildering array of templatized smart pointers of varying efficiency that can magically do something similar :-)
Comment 33 Stephan Bergmann 2013-11-04 08:32:30 UTC
(In reply to comment #32)
> clearly any usage of DELETEZ of a member in a
> destructor is bogus

even that need not be true in general
Comment 34 Commit Notification 2014-01-21 11:16:33 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h reference in oox



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 35 Commit Notification 2014-01-23 09:49:25 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h reference



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 36 Marcos Souza 2014-01-23 15:00:20 UTC
Hi guys,

I saw OSL_NETWORD, but it seems to bu unused inside the code. I'm missing something?

Can you guys give an example of how to use this...?

Thanks a lot since now!
Comment 37 Stephan Bergmann 2014-01-23 16:44:56 UTC
(In reply to comment #36)
> I saw OSL_NETWORD, but it seems to bu unused inside the code. I'm missing
> something?

Is part of the published URE API, so even if it is unused in LO itself, I wouldn't remove it.  (Not that it is likely used much in 3rd party code either, but it is IMO too little a code cruft to actually worry about.)

> Can you guys give an example of how to use this...?

As the documentation says, "swapping between host and network byte order."
Comment 38 Commit Notification 2014-01-24 16:10:22 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h reference in chart2



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 39 Commit Notification 2014-01-30 10:49:00 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h ref. in basic, cui, forms and writerfilter.



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 40 Commit Notification 2014-01-30 11:19:18 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h ref. in linguc., sfx2, starmath, svl and svx



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 41 Commit Notification 2014-01-30 11:24:45 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h ref. in vcl



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 42 Commit Notification 2014-01-30 11:26:11 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h ref. in vcl



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 43 Commit Notification 2014-01-30 11:27:32 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h ref. in svl



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 44 Commit Notification 2014-01-30 11:32:27 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h ref. in basic and formula



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 45 Commit Notification 2014-01-30 11:32:43 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h ref. in sfx2



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 46 Commit Notification 2014-01-30 11:34:03 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Removed unused solar.h ref. in svx



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 47 Commit Notification 2014-01-30 16:55:25 UTC
Marcos Paulo de Souza committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154: Remove SVBT8 from solar.h



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 48 Commit Notification 2014-02-12 07:24:41 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h ref in vcl, basctl, desktop..



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 49 Commit Notification 2014-02-15 04:59:42 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h reference in sc



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 50 Commit Notification 2014-02-15 05:06:14 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h reference in sc.



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 51 Commit Notification 2014-02-15 15:27:56 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h reference in sd.



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 52 Commit Notification 2014-02-17 10:26:49 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h reference in sw.



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 53 Alexandre Vicenzi 2014-02-19 02:32:25 UTC
Hi,

Any idea of where to put SVLIBRARY, or it can't be touched yet?
Comment 54 Stephan Bergmann 2014-02-19 09:19:12 UTC
(In reply to comment #6)
> not sure who had the crazy idea of hard-coding "lo" into dozens of library
> loading calls... but clearly that silly "lo" suffix serves no useful purpose
> and we should just get rid of it in the build system, in which case
> SVLIBRARY can be trivially replaced with SAL_MODULENAME.

See bug 67313 for a rationale why the "lo" suffix is useful after all.

(In reply to comment #53)
> Any idea of where to put SVLIBRARY, or it can't be touched yet?

In light of the above, I would still stick to comment 3.
Comment 55 Stephan Bergmann 2014-02-20 09:00:32 UTC
I just notice that this clean-up involves changing occurrences of sal_uLong to sal_uIntPtr, and I doubt that is a good idea.

The sal_uLong typedef was originally introduced to do a mass removal of tools/solar.h's ULONG (which clashed with a Windows typedef of the same name), without having to inspect all uses of ULONG and decide for an appropriate replacement type in each case---those inspections could be deferred for a later time by preserving the information about ULONG occurrences via the newly introduced sal_uLong (which happens to be a typedef to sal_uIntPtr because that happens to have the exact same underlying type as ULONG did).

So, occurrences of sal_uLong should not be blindly changed to sal_uIntPtr.  (Semantically, that often does not make sense, anyway.)  They should either be left alone or inspected to determine what other type they should actually be changed to (likely sal_uInt32, as the comment in tools/solar.h states).
Comment 56 Commit Notification 2014-02-20 09:54:47 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "master":

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

Revert "fdo#63154 Remove unused solar.h reference in sw."



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 57 Alexandre Vicenzi 2014-02-20 11:24:56 UTC
(In reply to comment #55)
> I just notice that this clean-up involves changing occurrences of sal_uLong
> to sal_uIntPtr, and I doubt that is a good idea.
> 
> The sal_uLong typedef was originally introduced to do a mass removal of
> tools/solar.h's ULONG (which clashed with a Windows typedef of the same
> name), without having to inspect all uses of ULONG and decide for an
> appropriate replacement type in each case---those inspections could be
> deferred for a later time by preserving the information about ULONG
> occurrences via the newly introduced sal_uLong (which happens to be a
> typedef to sal_uIntPtr because that happens to have the exact same
> underlying type as ULONG did).
> 
> So, occurrences of sal_uLong should not be blindly changed to sal_uIntPtr. 
> (Semantically, that often does not make sense, anyway.)  They should either
> be left alone or inspected to determine what other type they should actually
> be changed to (likely sal_uInt32, as the comment in tools/solar.h states).

Stephan,

I understand your point of view, and probably it's the best idea. It's wrong to put sal_uLong definition in sal/types.h?
Comment 58 Stephan Bergmann 2014-02-20 11:29:25 UTC
(In reply to comment #57)
> I understand your point of view, and probably it's the best idea. It's wrong
> to put sal_uLong definition in sal/types.h?

Yes, that's wrong.  sal_uLong is a "temporary" hack for a problem in code "above" the URE interface, so it should not find its way into the URE stable interface.

Looks like there is a handful things (SVLIBRARY. sal_uLong) that best stay in tools/solar.h for now, even if that means that tools/solar.h cannot be removed completely for now.
Comment 59 Commit Notification 2014-02-20 11:45:15 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h ref. in tools



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 60 Commit Notification 2014-02-20 20:39:52 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h reference in tools



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 61 Commit Notification 2014-02-21 09:25:31 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h reference in editeng



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 62 Alexandre Vicenzi 2014-02-27 01:42:55 UTC
(In reply to comment #56)
> Michael Stahl committed a patch related to this issue.
> It has been pushed to "master":
> 
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=4137f39d7b13c0ad2e649d7efb02ecfc14a31a25
> 
> Revert "fdo#63154 Remove unused solar.h reference in sw."
> 
> 
> 
> 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.

Maichael,

Can I get this patch and correct to reapply the changes I've made?
Or I need to create a new patch?
Comment 63 Michael Stahl (CIB) 2014-02-27 12:01:33 UTC
(In reply to comment #62)
> Can I get this patch and correct to reapply the changes I've made?
> Or I need to create a new patch?

something like "git revert 4137f39d7b13c0ad2e649d7efb02ecfc14a31a25"
followed by "git checkout HEAD~ files-that-contain-unwanted-changes",
then "git commit --amend" should probably work...
Comment 64 Alexandre Vicenzi 2014-02-27 12:20:20 UTC
Thanks Michael.
Comment 65 Commit Notification 2014-02-27 13:36:50 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 remove unused solar.h reference in tools



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 66 Commit Notification 2014-02-28 21:01:51 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h



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 67 Commit Notification 2014-03-09 16:26:57 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove unused solar.h reference in tools



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 68 Commit Notification 2014-03-09 16:52:21 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove old solar.h references



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 69 Commit Notification 2014-03-15 22:56:01 UTC
Alexandre Vicenzi committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63154 Remove some solar.h references



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 70 Robinson Tryon (qubit) 2015-12-14 06:33:51 UTC Comment hidden (obsolete)
Comment 71 jani 2016-02-17 07:18:10 UTC
Marcos: a polite ping, are you still working on this?
Comment 72 Chris Sherlock 2016-02-17 09:05:04 UTC
I wonder if we should split this one up a bit more - the scope for this one seems quite broad.

It's sort of hard to say when this one has finished because it's hard to tell what is and isn't being targetted. And we'd have the benefit of a few more easy hacks for new contributors to sink their teeth into.
Comment 73 Marcos Souza 2016-02-17 23:45:57 UTC
(In reply to jan iversen from comment #71)
> Marcos: a polite ping, are you still working on this?

Hi Jan,

No, I'm not. I stopped after trying to convert SVB<something> because they said it's not so ugly to have them.
Comment 74 Marcos Souza 2016-02-17 23:47:00 UTC
(In reply to Chris Sherlock from comment #72)
> I wonder if we should split this one up a bit more - the scope for this one
> seems quite broad.
> 
> It's sort of hard to say when this one has finished because it's hard to
> tell what is and isn't being targetted. And we'd have the benefit of a few
> more easy hacks for new contributors to sink their teeth into.

Chris, you're absolutely right. This bug is too generic for newcomers and they can be confused about how to start working on it. Splitting this into smaller tasks seems to be the best option.
Comment 75 jani 2016-02-18 06:48:46 UTC
After mail
Comment 76 Commit Notification 2017-12-11 08:56:39 UTC
ekuiitr committed a patch related to this issue.
It has been pushed to "master":

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

tdf#63154 removed some solar.h references

It will be available in 6.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 77 Michael Stahl (CIB) 2017-12-13 21:20:37 UTC
* i've redefined SVLIBRARY now similar to comment #3, see commit 68eb27e3bd5d536e7b00b7dc145ae4943b95bec9
* regarding DELETEZ i'd just stupidly expand all invocations, there's no harm in resetting a pointer if it's not necessary, might even make use-after-free easier to spot ... it looks like Noel has just started on this so we can assume it will be finished by next week
* for sal_uLong i've filed bug 114441

other than that, there is just the SVBT stuff left, don't think that can be easily removed.

so let's call this fixed for now.