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 ! :-)
Stephan might have some thoughts on this too :-)
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...?
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.
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 :)
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.
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.
@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 ...
> @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.
So, can I do as M Stahl said? Remove the SUFFIX variable and all usages and use only SAL_MODULENAME?
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.)
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.
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!
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 !
(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?)
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 !
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!
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.
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..
himanshu150891, I'm working in this bug... maybe you want to help with something? I'm replacing the _CR macros...
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
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.)
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.
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!
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 ].
(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.
> 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.
Am I wrong when I think that this bug is resolved? What's remaining?
> 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 :-)
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
Removing comma from whiteboard (please use a space to delimit values in this field) https://wiki.documentfoundation.org/QA/Bugzilla/Fields/Whiteboard#Getting_Started
Can I removed finally the DELETEZ macro too?
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 :-)
(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
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.
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.
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!
(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."
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Hi, Any idea of where to put SVLIBRARY, or it can't be touched yet?
(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.
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).
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.
(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?
(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.
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.
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.
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.
(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?
(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...
Thanks Michael.
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.
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.
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.
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.
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.
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup) [NinjaEdit]
Marcos: a polite ping, are you still working on this?
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.
(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.
(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.
After mail
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.
* 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.