881579 - Avoid multiple-inclusions of headers where possible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

If you have a canonical #ifndef wrapper in a header file, e.g.:

  #ifndef FOO_H__
  ...
  #endif

clang and GCC will avoid re-including the header if FOO_H__ is defined.  But
for this to work the #ifndef wrapper must have the above form (|#if
defined(FOO_H__)| is also allowed, and there can't be any tokens before or
afterwards (but whitespace and comments are ok).  

(See
http://gcc.gnu.org/onlinedocs/cpp/Once_002dOnly-Headers.html#Once_002dOnly-Headers
and http://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html for details.)

SpiderMonkey has numerous header files that violate this, and so get
multiply-included unnecessarily.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Use the canonical #ifndef wrapper form in SpiderMonkey headers so that GCC and clang can avoid including them more than once per compilation unit.

This patch fixes all the easy ones.  I've used the -H option to confirm the
relevant files are singly-included.  It reduces the number of files #include
during an SM build from 103,666 to 99,074.

There are some cases not yet fixed:

- Some inlines.h/-inl.h files are still multiply-included.  I think this is
  because jsinferinlines has all its #includes *before* the #ifndef wrapper!
  Fixing this is non-trivial.

- More importantly, all the $OBJDIR/dist/system_wrappers_js/*.h files lack an
  #ifndef wrapper.  I tried adding one but it caused compile errors.  I also
  tried #pragma once but that caused different compile errors.  I'm not sure
  what this means.

  It would be good to fix this one because some of these files are #included
  many times.  Furthermore, on my system some system headers don't have a
  canonical #ifndef wrapper (d'oh) and if the system_wrappers_js/*.h files were
  fixed that would prevent those system headers being multiply-included.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 760682 [details] [diff] [review]
Use the canonical #ifndef wrapper form in SpiderMonkey headers so that GCC and clang can avoid including them more than once per compilation unit.

Review of attachment 760682 [details] [diff] [review]:
-----------------------------------------------------------------

No good deed goes unpunished...

Comment 4

[:Benjamin Peterson]

Comment on attachment 760682 [details] [diff] [review]
Use the canonical #ifndef wrapper form in SpiderMonkey headers so that GCC and clang can avoid including them more than once per compilation unit.

Review of attachment 760682 [details] [diff] [review]:
-----------------------------------------------------------------

I see our naming convention of these defines is inconsistent.

gc/StoreBuffer.h -> jsgc_storebuffer_h___
ion/BaselineCompiler.h -> jsion_baseline_compiler_h__
vm/Stack.h -> Stack_h__

For bonus points, you could standardize that.

Comment 5

[Nicholas Nethercote [inactive]]

> For bonus points, you could standardize that.

I'll do it as a follow-up.  I've emailed the JS internals list to get consensus on the exact form.

(Nb: A cursory inspection finds that gc/FindSCCs-inl.h doesn't even have a #ifndef wrapper!  Yuk.  Bring on bug 880088.

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efbe3b2ea841

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/efbe3b2ea841