Moving away from tools module

Sun Apr 30 23:05:22 UTC 2017

>> On 28 Apr 2017, at 8:22 pm, Michael Stahl <mstahl at redhat.com> 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 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?

>> -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.

Oops, fair point. But it is something useful enough to be needed in the VCL, and in modules that rely on the VCL already. Nothing else seems to need it.

>> - 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.

I'll check.

>> 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. 

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?

Chris