#include file naming convention (followup to namespaces)

[Benjamin Smedberg]

Now that we're using namespaces, I think we should also consider how we
#include headers. In particular:

If a header declares a class mozilla::Mutex, we should #include
"mozilla/Mutex.h". Otherwise we risk header naming conflicts with other
projects which might have a Mutex.h.

In addition, I'd also like to flatten dist/include, so that all of our
header files are installed directly to dist/include, rather than to
dist/include/$(MODULE). This means that we would no longer need REQUIRES.

I originally proposed this in bug 398573 and dbaron proposed a wider
discussion (and then I got distracted for a year).

The cost of this change:

* it would no longer be explicit which headers belong to which modules
* it might be easier to add "bad dependencies" because they would no longer
require changes to REQUIRES in the makefiles.

The benefits:

* Nobody has to maintain explicit REQUIRES lists
* It's a lot easier to implement #include "namespace/Header.h"

--BDS

[Arpad Borsos]

> If a header declares a class mozilla::Mutex, we should #include
> "mozilla/Mutex.h". Otherwise we risk header naming conflicts with other
> projects which might have a Mutex.h.

As every class will end up in the mozilla:: namespace, writing
#include "mozilla/Class.h" seems too redundant to me.
This should only be done for sub namespaces like mozilla::storage with
#include "storage/Class.h"

> In addition, I'd also like to flatten dist/include, so that all of our
> header files are installed directly to dist/include, rather than to
> dist/include/$(MODULE). This means that we would no longer need REQUIRES.

Please do this.

[Benjamin Smedberg]

On 4/8/09 5:00 AM, Arpad Borsos wrote:
>> If a header declares a class mozilla::Mutex, we should #include
>> "mozilla/Mutex.h". Otherwise we risk header naming conflicts with other
>> projects which might have a Mutex.h.
>
> As every class will end up in the mozilla:: namespace, writing
> #include "mozilla/Class.h" seems too redundant to me.
> This should only be done for sub namespaces like mozilla::storage with
> #include "storage/Class.h"

This doesn't solve the problem where #include "Mutex.h" might have file
naming conflicts with other projects (for example, mheaders in
/usr/include). The Google C++ guidelines use the full namespace include path
and I think it makes it much clearer where you might find relevant headers. See

http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/google_breakpad/common/minidump_format.h#66

In addition, it will become much easier for embedders to include headers the
same way Mozilla does, which has been a pain point for them.

--BDS

[Vladimir Vukicevic]

On 4/8/09 5:54 AM, Benjamin Smedberg wrote:
> On 4/8/09 5:00 AM, Arpad Borsos wrote:
>>> If a header declares a class mozilla::Mutex, we should #include
>>> "mozilla/Mutex.h". Otherwise we risk header naming conflicts with other
>>> projects which might have a Mutex.h.
>>
>> As every class will end up in the mozilla:: namespace, writing
>> #include "mozilla/Class.h" seems too redundant to me.
>> This should only be done for sub namespaces like mozilla::storage with
>> #include "storage/Class.h"
>
> This doesn't solve the problem where #include "Mutex.h" might have file
> naming conflicts with other projects (for example, mheaders in
> /usr/include). The Google C++ guidelines use the full namespace include path
> and I think it makes it much clearer where you might find relevant headers. See

I'd suggest mozilla::storage::Class ->

#include "mozilla/storage/Class.h"

With OSX-style helper includes, e.g.

#include "mozilla/Storage.h"

which would #include either all or the most common pieces of
mozilla/storage/* as appropriate for the module. Most developers don't
actually know exactly which #include files they need (I sure don't), and
end up just copying sets of include files from other files until things
work. Having clear module-level includes would greatly simplify the
tops of our source files, IMO.

- Vlad

[Benjamin Smedberg]

On 4/8/09 2:51 PM, Vladimir Vukicevic wrote:

> I'd suggest mozilla::storage::Class ->
>
> #include "mozilla/storage/Class.h"
>
> With OSX-style helper includes, e.g.
>
> #include "mozilla/Storage.h"

Absolutely. The ability to include an entire module will also make it much
easier to experiment with precompiled headers.

--BDS

[Vladimir Vukicevic]

Hmm.. we could start setting up the module-level headers now, even
without namespaces or anything like that... doing so would make it
easier to convert later on as well.

What's the best way to tackle that? Start by coming up with a list of
modules that should have their own module-level include files?

What granularity should this be? That is, should it be #include
"mozilla/XPCOM.h", or should it be "mozilla/xpcom/Components.h" ? I'd
say XPCOM.h; that holds well for, say, Graphics.h, but maybe doesn't
hold as well for Content.h and Layout.h ?

What do we do about modules with lots of IDL interfaces? Just include
the most common ones, and then make specific files include the more
one-off ones? (I'm thinking of the interfaces for all the dom elements,
say.)

- Vlad

[L. David Baron]

On Monday 2009-04-13 18:10 -0700, Vladimir Vukicevic wrote:
> Hmm.. we could start setting up the module-level headers now, even
> without namespaces or anything like that... doing so would make it
> easier to convert later on as well.

I think we should do experiments about effects on compilation time
before we rush off in any direction here. In the past, reducing
header dependencies has led to significant improvements in
compilation time (probably both because of I/O and because of the
parse time).

-David

--
L. David Baron http://dbaron.org/
Mozilla Corporation http://www.mozilla.com/

[Benjamin Smedberg]

On 4/13/09 9:10 PM, Vladimir Vukicevic wrote:

> What's the best way to tackle that? Start by coming up with a list of
> modules that should have their own module-level include files?

I have a patch in the works for mozilla/XPCOM.h and the build system
infrastructure necessary to install headers and flatten the hierarchy.

> What granularity should this be? That is, should it be #include
> "mozilla/XPCOM.h", or should it be "mozilla/xpcom/Components.h" ? I'd
> say XPCOM.h; that holds well for, say, Graphics.h, but maybe doesn't
> hold as well for Content.h and Layout.h ?

I'm going to do mozilla/XPCOM.h, but exclude the obsolete headers and a few
of the more obscure public headers such as nsIWindowsRegKey.h (which brings
in windows.h).

For really big modules, perhaps smaller divisions would be appropriate... I
don't know.

> What do we do about modules with lots of IDL interfaces? Just include
> the most common ones, and then make specific files include the more
> one-off ones? (I'm thinking of the interfaces for all the dom elements,
> say.)

For XPCOM I'm going to include them all.

As for dbaron's concern about build time: if we have to make a tradeoff
between ease of reading and learning our code and some build-time increase,
I think we should go with ease of learning and take small build-time hits.
Once we have module-level includes, we can buy back time by using PCH pretty
easily.

--BDS

[Boris Zbarsky]

Benjamin Smedberg wrote:
> As for dbaron's concern about build time: if we have to make a tradeoff
> between ease of reading and learning our code and some build-time increase,
> I think we should go with ease of learning and take small build-time hits.

If they're small hits, yes. What do we do if they're big hits?

In any case, we should measure to make sure the hits are indeed small.

-Boris