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

On 09/17/2012 11:32 AM, Norbert Thiebaud wrote:
> On Mon, Sep 17, 2012 at 3:27 AM, Stephan Bergmann <sbergman@redhat.com> wrote:
> > sal/types.h just happens to be a header that is needed in many compilation
> > units.
>
> any compilation unit that use any scalar type sal_* woudl need it...
> iow all of them practically...
>
> cppuhelper/source/findofficepath.c
> tools/source/misc/pathutils.cxx
> extensions/source/nsplugin/source/so_env.cxx
>
> are the only source I could find that include config.h and not
> types.h... and it is not clear at all that it was deliberate.
> which impacted 2 static library: libnpsoenv.a and libooopathutils.a
>
> ucbhelper/source/client/contenbrooker.cxx was a false postive as the
> content of if is excluded on non ANDROID
> vcl/source/app/dbggui.cxx showed-up was a false positive since I did
> not build in DBG_UTIL mode... ortherwise sal/types.h would have been
> included

Yes, but what is your point?  (My point is that (a) there is a rationale 
for inclusion of sal/config.h at the top of each other file, and (b) 
there is some header, sal/types.h, that happens to be included 
~everywhere, and that (a) and (b) are rather unrelated.)

> > and for another,
> > I think there currently is nothing in config.h that makes it extremely
> > problematic if it happens to not be included first thing into some
> > compilation unit
>
> That is a flaw, when you want a heder to be first, you got to make
> sure that you can't forget about it :-)

How would you make sure?

> > , so enforcing it by modifying hundreds of files probably is
> > not worth it right now.)
>
> Well, it is like saying... we have thousand of warnings... so
> contemplating -WError is not worth it...
> that is a self-fullfilling prophecy...

No, not at all.  What I'm saying is that the missing includes of 
sal/config.h do not cause any harm right now, so I see no need to bother 
with that right now.

> > I could understand the desire for some lo.h from the point of view of
> > performance (though the relevant sub-headers will probably manage to stay in
> > main memory anyway) or ease-of-maintenance, but what do you mean with
> > "flexibility and certainty"?
>
> having a header that is  guaranteed to be included in every source
> first (I mean source not header)

...which is what sal/config.h is there for.  (If you want to add 
something to it for which it would actually cause harm if sal/config.h 
is not included first thing everywhere, then we should fix the latter.)

> > > 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.
> >
> > Why?  The includes of assert.h in headers pertain to uses of assert in
> > (inline) functions defined in those headers.  I see no need to complicate
> > things by building a protocol on top of plain Standard C assert.h here.
>
> Just because the 'Standard' allow you to shoot yourself in the foot
> does not mean it is a good idea.

But I see no shooting going on here that we would need to defend 
ourselves against?

> > > 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...
> >
> >
> > ...measurably?
>
> yes measurably 0.076875 milli-seconds per #include <assert.h> on my linux box.
>
> or to put in another way: 0.15% of the total build time  (6 second
> over 4 cpu, out of 17 minutes elapsed)
>
> ****
> n_th@tpa10 ~ ()$ cat test_assert.c
>
> #include <assert.h>
>
> #ifdef MANY_ASSERT
> #include "many_assert.h" // 80K include assert.h

Are you sure about that 80K?  For me, e.g., "grep -Flwr rtl/ustring.hxx 
workdir/unxlngx6/Dep | wc -l" reports 8425, so the number of actual 
includes of the rtl/ustring.hxx file during the build should be ca. 4200 
(as each header appears twice at least in CxxObject/*.d files).

Anyway, even if reducing the number of #include <cassert> in the code 
gives a slight compilation speedup, I see no reason why this one 
specific case, cassert, should be addressed by replacing all those 
individual #includes with a single one in some strategic header, while 
many other cases (cstddef, cstring, ...) would not.

Stephan