Re: Moving away from tools module

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