Re: Moving away from tools module
On 01.05.2017 01:05, Chris Sherlock wrote: >>> On 28 Apr 2017, at 8:22 pm, Michael Stahl wrote: >>> >>> On 28.04.2017 00:36, Chris Sherlock wrote: > ... >>> - debug.hxx and diagnose_ex.hxx - seems more suited to the RTL... >> >> these mostly contain deprecated macros whose usage should be replaced with >> either assert or macros from sal/log.hxx - but it requires >> case-by-case decision as to what to replace each call with - we used to have >> an easy-hack for that but it was disabled because inexperienced developers >> rarely got it right. > > I'm happy to try to tackle this. I'd put the changes up on gerrit, would > anybody be willing to review the changes and push me in the right direction > if I did so? yes, with some caveats: the most valuable replacement is towards assert(), which grabs the attention of developers by aborting, so the occasional new assert() that triggers will be fixed. however, if the build gets into a state where too many assert() are triggered, then developers will complain that they can't fix the bugs they want to fix because of all these new asserts and demand a way to disable them. in particular, when you add an assert() "make check" must pass completely (no cheating by reducing coverage with --without-java!) because developers and CI rely on that, and if it doesn't pass you need to debug the failure and fix whatever causes it. but our "make check" code coverage is only around 40%, so there is a lot (particularly UI) code that make check won't execute; QA sometimes files bugs when they use debug builds, and the filter "crash-test" that is run at least once a week also tends to find files that trigger asserts. so, we should convert the legacy assertions slowly and file by file, deciding case by case if it should be assert or SAL_WARN or SAL_INFO, and always make sure that "make check" passes so this is going to take a long time. this is also why we have argued against an "automatic" replacement of the legacy assertions with SAL_WARN; the fact that a legacy assertion is used tells us that nobody has thought if it should be an assert or just a SAL_WARN, while a SAL_WARN is assumed to be an already decided case. that said, there are some cases where it's really obvious that assert is the right answer, for example: OSL_ENSURE(pointer, "the pointer is null"); pointer->foo; following the legacy assert, the pointer is unconditionally dereferenced, so that is going to segfault anyway, so we know it's not triggered. >>> There are quite a few headers, but as you can see most look to be more >>> appropriately moved to another module.. This would help streamline our >>> module dependencies... >> >> i really don't see the point of this. the tools module is primarily a toxic >> waste dump, and distributing the toxic waste across all the other >> modules does not look like an improvement to me. better to remove the toxic >> waste from our git repo and dump it in some landfill where nobody lives, or >> at least nobody that we know :) > > That does seem to be the consensus :-) however, at least one of the classes > is used extensively through our codebase, and I'm not sure what would replace > it.., I'm speaking of SvStream. SvStream is not as bad as it used to be after Noel replaced the ridiculous overloaded serialization functions with WriteUInt8, WriteUInt16 etc., and it's using osl_File now instead of re-implementing that abstraction. anyway, we clearly have made substantial progress in tools over the years: > git diff --stat libreoffice-3.3.0.4.. tools include/tools 317 files changed, 19001 insertions(+), 74121 deletions(-) ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Moving away from tools module
On 05/01/2017 01:05 AM, Chris Sherlock wrote: On 28 Apr 2017, at 8:22 pm, Michael Stahl wrote: i really don't see the point of this. the tools module is primarily a toxic waste dump, and distributing the toxic waste across all the other modules does not look like an improvement to me. better to remove the toxic waste from our git repo and dump it in some landfill where nobody lives, or at least nobody that we know :) That does seem to be the consensus :-) however, at least one of the classes is used extensively through our codebase, and I'm not sure what would replace it.., I'm speaking of SvStream. Whilst it's not useless or deprecated, I wonder if it was placed in tools because nobody was quite sure where else to put it? I think you're looking at it from the wrong angle. Long ago, tools used to be the natural choice where to put generic low-level stuff (it being the lowest module in the hierarchy). That's how many of the tools include files came about. Things have changed since them. Some parts of tools don't make much sense any more, some parts are known to be so bad that they shouldn't be used any more, some parts have been superseded by facilities elsewhere (where that "elsewhere" is often just due to historical reasons, too), etc. And some parts (like SvStream) are likely to stay with us for quite some time still. So while it makes sense to clean away parts that are no longer needed and have good replacements elsewhere, it IMO makes no sense to e.g. move existing functionality from tools to o3tl. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Moving away from tools module
Chris Sherlock wrote: > So this seems to be deprecated, and if we need *true* BigInt support > we are better off with something like gmp? > Nah, what Michael is suggesting to check all call sites & perhaps just move them to sal_Int64 where possible - > Would this potentially be the candidate for an easy hack? > Yeah, why not. Do some sample changes & see, I'd say. And let's leave the gmp discussion until the easier cases are done... > There seem to be some ancillary functions not included in basegfx, > is it worthwhile adding these to the color tools in basegfx? > Needs looking into the details - I would suspect ~none of that is really needed outside vcl these days. > Hey :-) yeah, Rectangle is quite remarkable... perhaps though this > might be something for an easy hack? > > Rectangle, Point, etc are completely intertwined with the VCL > though, is it worthwhile degunking the VCL module of the use of > these classes? > Hmm, git grep tools::Rectangle | wc -l -> 9224 - so that's _much_ more than just vcl. Would be great if someone could tackle that, but me thinks that needs some serious planning / prototyping before. Especially the invalidate/update/size calculations are all over the place, and particularly delicate due to this bottom/right line included/excluded subleties. Cheers, -- Thorsten signature.asc Description: Digital signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Moving away from tools module
>> On 28 Apr 2017, at 8:22 pm, Michael Stahl wrote: >> >> On 28.04.2017 00:36, Chris Sherlock wrote: ... >> I cleaned up ErrorHandler, but I thought I'd ask about the other headers >> here: >> >> - b3dtrans.hxx - I'm assuming the b3d bit means basegfx 3D transformations. >> I've >> submitted https://gerrit.libreoffice.org/#/c/37030/ to move it into basegfx >> but unfortunately it's failing on Windows and I don't currently have a >> working system to determine why the linker is erroring out. > > i'm curious why this isn't already in basegfx - perhaps it's some sort of > compatibility layer and there are already equivalent functions in basegfx or > somewhere? pretty sure i've seen some coordinate transformation class > somewhere, and it wasn't this one... obviously it can't be moved to basegfx > as-is since it uses tools Rectangle. OK, so this one needs some more research on my end, appreciate the feedback :-) Not entirely sure why I'm getting the linker error on Windows though, anyone know why this is occurring? >> - bigint.hxx and fract.hxx - seems to be more appropriate to the sal module, >> possibly the RTL? Great to get thoughts of others on this one > > BigInt *predates* availability of 64-bit integers - i bet many of its users > would be fine with a sal_Int64 and don't need arbitrary precision. > it even uses "short"s to represent digits, this is 16-bit code... So this seems to be deprecated, and if we need *true* BigInt support we are better off with something like gmp? Would this potentially be the candidate for an easy hack? > Fraction is nowadays basically a pImpl wrapper around some boost class that > pulls in 50k lines of amazing template metaprogramming headers so can't be > included everywhere. Possibly not something that we want to "solve", but if this is the case, then this one seems to be a genuinely useful solution to a problem causing compilations to take longer than necessary. But as it appears that tools is our embarrassing colleagues who we don't want to work with but are forced to out of necessity, maybe this might be better off in o3tl? >> - color.hxx and colordata.hxx - should that migrate to basegfx also? > > that already has a color in include/basegfx/color/ ... There seem to be some ancillary functions not included in basegfx, is it worthwhile adding these to the color tools in basegfx? > >> - gen.hxx - includes Pair (shouldn't that be deprecated to std::pair?), >> Point, Size, Range, Selection and Rectangle >> - poly.hxx - polygons >> - line.hxx - lines >> >> gen.hxx, lines.hxx and poly.hxx all are drawing primitives, wouldn't these >> be better off in basegfx? > > these all already exist in basegfx, the problem is that nobody has time to > convert the existing usages of tools classes to basegfx ones, which > is tricky because some of the tools classes are just amazingly badly designed > (see for example documentation of Rectangle class). Hey :-) yeah, Rectangle is quite remarkable... perhaps though this might be something for an easy hack? Rectangle, Point, etc are completely intertwined with the VCL though, is it worthwhile degunking the VCL module of the use of these classes? So I guess basegfx is something I should spend some time focussing on in my book. >> - config.hxx - where would this go? I was thinking the RTL, but it also >> seems like something the VLC might do... > > this looks like a class to handle config files - surely something like this > already exists in sal/rtl, for rtl::Bootstrap reading its stuff... Yup, rtl::Bootstrap does this already. Actually, I looked over some of the code in there and it seems that it might need a little love around variable parsing at some point, but works well enough now that it's not a concern to anyone. >> - contr.hxx - seems better suited in the svx module > > this defines 4 magic constants - why is this still in use? probably a remnant > of the tools-container -> STL conversion of 1897 or when it was? I suspect that to be the case, so in that case maybe the focus should be to find the bits that use this and migrate them to use the standard template library. >> - time.hxx, date.hxx, datetime.hxx and datetimeutils.hxx - these all seem to >> be better suited to the SAL, and actually should we consider moving to >> chrono? > > there is a boost datetime thing too, we were looking at that some years ago > but i forgot why we decided not to use it. perhaps it pulls in 50k > loc of headers everywhere and makes compile times go through the roof like > boost stuff is wont to do. I was wondering about that. Boost is pretty amazing, but are parts of it bloated or just implemented inefficiently? I'm not clear about this unfortunately... >> - debug.hxx and diagnose_ex.hxx - seems more suited to the RTL... > > these mostly contain deprecated macros whose usage should be replaced with > either assert or macros from sal/log.hxx - but it requires > case-by-case dec
Re: Moving away from tools module
On 28.04.2017 00:36, Chris Sherlock wrote: > Hi all, > > As I'm researching my book on the guts of LibreOffice, I notice that > tools predates the SAL. In fact it's a bit of a hodge-lodge of files, > many of which look like they should probably migrated into other modules. > > I cleaned up ErrorHandler, but I thought I'd ask about the other headers > here: > > - b3dtrans.hxx - I'm assuming the b3d bit means basegfx 3D > transformations. I've > submitted https://gerrit.libreoffice.org/#/c/37030/ to move it into > basegfx but unfortunately it's failing o Windows and I don't currently > have a working system to determine why the linker is erroring out. i'm curious why this isn't already in basegfx - perhaps it's some sort of compatibility layer and there are already equivalent functions in basegfx or somewhere? pretty sure i've seen some coordinate transformation class somewhere, and it wasn't this one... obviously it can't be moved to basegfx as-is since it uses tools Rectangle. > - bigint.hxx and fract.hxx - seems to be more appropriate to the sal > module, possibly the RTL? Great to get thoughts of others on this one BigInt *predates* availability of 64-bit integers - i bet many of its users would be fine with a sal_Int64 and don't need arbitrary precision. it even uses "short"s to represent digits, this is 16-bit code... Fraction is nowadays basically a pImpl wrapper around some boost class that pulls in 50k lines of amazing template metaprogramming headers so can't be included everywhere. > - color.hxx and colordata.hxx - should that migrate to basegfx also? that already has a color in include/basegfx/color/ ... > - gen.hxx - includes Pair (shouldn't that be deprecated to std::pair?), > Point, Size, Range, Selection and Rectangle > - poly.hxx - polygons > - line.hxx - lines > > gen.hxx, lines.hxx and poly.hxx all are drawing primitives, wouldn't > these be better off in basegfx? these all already exist in basegfx, the problem is that nobody has time to convert the existing usages of tools classes to basegfx ones, which is tricky because some of the tools classes are just amazingly badly designed (see for example documentation of Rectangle class). > - config.hxx - where would this go? I was thinking the RTL, but it also > seems like something the VLC might do... this looks like a class to handle config files - surely something like this already exists in sal/rtl, for rtl::Bootstrap reading its stuff... > - contr.hxx - seems better suited in the svx module this defines 4 magic constants - why is this still in use? probably a remnant of the tools-container -> STL conversion of 1897 or when it was? > - cpuid.hxx - checks for SSE2 support, surely something for the SAL? > > - time.hxx, date.hxx, datetime.hxx and datetimeutils.hxx - these all > seem to be better suited to the SAL, and actually should we consider > moving to chrono? there is a boost datetime thing too, we were looking at that some years ago but i forgot why we decided not to use it. perhaps it pulls in 50k loc of headers everywhere and makes compile times go through the roof like boost stuff is wont to do. > - debug.hxx and diagnose_ex.hxx - seems more suited to the RTL... these mostly contain deprecated macros whose usage should be replaced with either assert or macros from sal/log.hxx - but it requires case-by-case decision as to what to replace each call with - we used to have an easy-hack for that but it was disabled because inexperienced developers rarely got it right. > -extendApplicationEnvironment - seems to be a candidate for the OSL... sal/osl definitely shouldn't be setting a URE_BOOTSTRAP variable - or any URE layer code for that matter. > - fldunit.hxx and fontenum.hxx - both seems to be more appropriate in > the VCL it appears so, but likely you'll find these to be used in modules that don't want to depend on VCL. > There are quite a few headers, but as you can see most look to be more > appropriately moved to another module.. This would help streamline our > module dependencies... i really don't see the point of this. the tools module is primarily a toxic waste dump, and distributing the toxic waste across all the other modules does not look like an improvement to me. better to remove the toxic waste from our git repo and dump it in some landfill where nobody lives, or at least nobody that we know :) ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Moving away from tools module
Sent from my iPhone > On 28 Apr 2017, at 7:07 pm, Eike Rathke wrote: > > Hi Chris, > >> On Friday, 2017-04-28 08:36:19 +1000, Chris Sherlock wrote: >> >> - time.hxx, date.hxx, datetime.hxx and datetimeutils.hxx - these all seem to >> be better suited to the SAL, and actually should we consider moving to >> chrono? > > chrono is not a replacement as it doesn't offer the functionality of > tools::Date and DateTime, only for Time. > > I don't care in which module the source code lives or what implements > it, but I definitely want to keep the existing functionality. > > Problem with moving anything to sal is that we then guarantee stable API > and even ABI to some degree as sal is part of the SDK, or have LibO only > interfaces like with OUString. > > Eike > > -- > LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. > GPG key 0x6A6CD5B765632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 > 2D3A > Care about Free Software, support the FSFE https://fsfe.org/support/?erack That's a fair point :-) I believe there is a proposal to include Howard Hinnant's very comprehensive calendaring and Timezone library into the C++ standard: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0355r2.html Nice things about this is that the app dusting library can process the tz database, and a number of other interesting things besides: https://github.com/HowardHinnant/date/blob/master/README.md Chris___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Moving away from tools module
> On 28 Apr 2017, at 10:30 am, Bjoern Michaelsen > wrote: > > Hi, > >> On Fri, Apr 28, 2017 at 08:36:19AM +1000, Chris Sherlock wrote: >> Thoughts? > > tools is the "this really need to die" module. The only place anything from > there should ever move to is oblivion. > > Best, > > Bjoern Oh! But doesn't SvStream exist in tools? Chris ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Moving away from tools module
Hi Chris, On Friday, 2017-04-28 08:36:19 +1000, Chris Sherlock wrote: > - time.hxx, date.hxx, datetime.hxx and datetimeutils.hxx - these all seem to > be better suited to the SAL, and actually should we consider moving to chrono? chrono is not a replacement as it doesn't offer the functionality of tools::Date and DateTime, only for Time. I don't care in which module the source code lives or what implements it, but I definitely want to keep the existing functionality. Problem with moving anything to sal is that we then guarantee stable API and even ABI to some degree as sal is part of the SDK, or have LibO only interfaces like with OUString. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GPG key 0x6A6CD5B765632D3A - 2265 D7F3 A7B0 95CC 3918 630B 6A6C D5B7 6563 2D3A Care about Free Software, support the FSFE https://fsfe.org/support/?erack signature.asc Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Moving away from tools module
Hi, On Fri, Apr 28, 2017 at 08:36:19AM +1000, Chris Sherlock wrote: > Thoughts? tools is the "this really need to die" module. The only place anything from there should ever move to is oblivion. Best, Bjoern ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
Moving away from tools module
Hi all, As I'm researching my book on the guts of LibreOffice, I notice that tools predates the SAL. In fact it's a bit of a hodge-lodge of files, many of which look like they should probably migrated into other modules. I cleaned up ErrorHandler, but I thought I'd ask about the other headers here: - b3dtrans.hxx - I'm assuming the b3d bit means basegfx 3D transformations. I've submitted https://gerrit.libreoffice.org/#/c/37030/ to move it into basegfx but unfortunately it's failing o Windows and I don't currently have a working system to determine why the linker is erroring out. - bigint.hxx and fract.hxx - seems to be more appropriate to the sal module, possibly the RTL? Great to get thoughts of others on this one - color.hxx and colordata.hxx - should that migrate to basegfx also? - gen.hxx - includes Pair (shouldn't that be deprecated to std::pair?), Point, Size, Range, Selection and Rectangle - poly.hxx - polygons - line.hxx - lines gen.hxx, lines.hxx and poly.hxx all are drawing primitives, wouldn't these be better off in basegfx? - config.hxx - where would this go? I was thinking the RTL, but it also seems like something the VLC might do... - contr.hxx - seems better suited in the svx module... - cpuid.hxx - checks for SSE2 support, surely something for the SAL? - time.hxx, date.hxx, datetime.hxx and datetimeutils.hxx - these all seem to be better suited to the SAL, and actually should we consider moving to chrono? - debug.hxx and diagnose_ex.hxx - seems more suited to the RTL... -extendApplicationEnvironment - seems to be a candidate for the OSL... - fldunit.hxx and fontenum.hxx - both seems to be more appropriate in the VCL There are quite a few headers, but as you can see most look to be more appropriately moved to another module. This would help streamline our module dependencies... Thoughts? Chris Sent from my iPhone___ LibreOffice mailing list LibreOffice@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice
