sal/config.g sal/types.h and sal/osl/rtl includes in general

[Norbert Thiebaud]

Stephan,

in commit e9689e4fdcf876e7bcaf564e060a3512e0fe9ef3

you said:

Revert "saldllapi.h is really not included outside of sal itself"
This reverts commit 2dfe34ce0efef6ec0412130a32f755657710363d:

* "not every header needs to include sal/types.h"

* sal/config.h is the header to always include first (not sal/types.h)

that is a nice theory but:

n_th at tpa10 /lo/g_core/workdir/unxlngx6.pro/Dep (master)$ for f in
$(find /lo/g_core/sal/inc -type f) ; do echo "file $f : $(grep -R
"/${f/\/lo\/g_core\/sal\/inc\//}[^a-z]" * | wc -l)" ; done
...
file /lo/g_core/sal/inc/sal/config.h : 25887
...
file /lo/g_core/sal/inc/sal/types.h : 25873
...

so somehow somewhere there are 14 object that manage to include
config.h without including types.h out of 25887...
So what is the point of including <sal/config.h> _first_
and then have to include <sal/types.h>  which in turn include
sal/config.h (and other generally useful bits) anyway ?

furthermore:
n_th at tpa10 /lo/g_core (master)$ git grep "sal/types.h" | wc -l
1323
n_th at tpa10 /lo/g_core (master)$ git grep "sal/config.h" | wc -l
659
n_th at tpa10 /lo/g_core (master)$

so not only sal/config.h must be included first is not really
enforced.. but type.h is actually much more popular...


What I'm really aiming at, is to have a lo.h header (in solenv/inc for
instance) that one can/should really use in every _source_ (not
header) as a first include, to take care of most sal/osl/rtl
boilerplate includes...

As a data point, here is the list of include in sal, with the number
of time they show up as a dependency of an object  (linux build, only
the one with 10K+ dep shown)

file /lo/g_core/sal/inc/rtl/ref.hxx : 11497
file /lo/g_core/sal/inc/sal/mathconf.h : 15211
file /lo/g_core/sal/inc/rtl/math.hxx : 15238
file /lo/g_core/sal/inc/rtl/math.h : 15244
file /lo/g_core/sal/inc/osl/thread.h : 15900
file /lo/g_core/sal/inc/osl/time.h : 17495
file /lo/g_core/sal/inc/rtl/ustrbuf.hxx : 18191
file /lo/g_core/sal/inc/osl/endian.h : 18728
file /lo/g_core/sal/inc/rtl/ustrbuf.h : 19530
file /lo/g_core/sal/inc/osl/getglobalmutex.hxx : 22357
file /lo/g_core/sal/inc/rtl/instance.hxx : 22378
file /lo/g_core/sal/inc/osl/doublecheckedlocking.h : 22381
file /lo/g_core/sal/inc/osl/mutex.hxx : 22625
file /lo/g_core/sal/inc/osl/mutex.h : 22639
file /lo/g_core/sal/inc/rtl/alloc.h : 23015
file /lo/g_core/sal/inc/rtl/oustringostreaminserter.hxx : 23930
file /lo/g_core/sal/inc/rtl/ustring.hxx : 23964
file /lo/g_core/sal/inc/rtl/stringutils.hxx : 24015
file /lo/g_core/sal/inc/rtl/string.hxx : 24022
file /lo/g_core/sal/inc/rtl/memory.h : 24053
file /lo/g_core/sal/inc/rtl/ustring.h : 24076
file /lo/g_core/sal/inc/rtl/string.h : 24137
file /lo/g_core/sal/inc/rtl/textcvt.h : 24196
file /lo/g_core/sal/inc/osl/interlck.h : 24208
file /lo/g_core/sal/inc/rtl/textenc.h : 24225
file /lo/g_core/sal/inc/osl/diagnose.h : 24244
file /lo/g_core/sal/inc/sal/log.hxx : 25529
file /lo/g_core/sal/inc/sal/detail/log.h : 25871
file /lo/g_core/sal/inc/sal/types.h : 25873
file /lo/g_core/sal/inc/sal/config.h : 25887
file /lo/g_core/sal/inc/sal/macros.h : 25890

right now a lot of them get pulled-in because somewhere along the
includes there is a #include <rtl/ustring.hxx>
but the include-strategy is everything but a strategy... it is more a
"sal/* osl/* rtl/* get sprinkled all over, in cxx or hxx, until the
darn thing compile" - strategy.

having a common, mandatory header that is actually included in every
source and only in source of the product (above sal), would provide a
much better visibility over how sal-related stuff get included, and
much more flexibility and certainty when doing product wise changes...


in commit 659c6a363bf84bd2520042ba80bc507763b57b78
you said:

   For one, assert.h is designed to be includeable multiple times with changing
    NDEBUG settings, so it is not robust to include it early in
sal/macros.h with
    "correct" NDEBUG settings and potentially include it again later.

you are right... but that odd behavior of assert.h is actually a
reason to hunt down random #include of assert.h, especially in
headers, and centralize its use.
for instance
sal/inc/osl/thread.hxx:#include <cassert>
sal/inc/rtl/strbuf.hxx:#include <cassert>
sal/inc/rtl/string.hxx:#include <cassert>
sal/inc/rtl/ustrbuf.hxx:#include <cassert>
sal/inc/rtl/ustring.hxx:#include <cassert>
sal/textenc/unichars.hxx:#include <cassert>

and, see above, these guys are included _a lot_ so assert.h get
defined/un-defined/redefined a lot too (just with the 6 include above
we are talking about 85K times. or somewhere liek 4 times per
source...
by centralyzing it at some choke point (sal/config.h if you want) and
hunting down random include of assert.h we actually improve things...


you said:
"   for another,
    there is #ifndef NDEBUG code providing functionality used exclusively within
    assert calls, which must be compiled with the same NDEBUG-setting as the
    relevant #include <assert.h>."

Indeed that is a good point. I was concerned with the other way
around... which is that some code behavior would be changed by NDEBUG
while having nothing to do with assert()...
my goal being to be able to take the assert() in release code while
not changing anything else in the behavior of the product....

I guess both abuse of NDEBUG should be investigated... but your method
is probably safer....


Norbert