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 <mstahl@redhat.com> 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(-)