1011454 - check_spidermonkey_style.py incorrectly considers an -inl.h file a module header

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Lars T Hansen [:lth]]

Consider this set of include files in ForkJoinNursery.cpp (cf bug 933313):

#include "gc/ForkJoinNursery.h"
#include <inttypes.h>
#include "prmjtime.h"
#include "gc/Heap.h"
#include "vm/ArrayObject.h"
#include "vm/ForkJoin.h"
#include "vm/TypedArrayObject.h"
#include "jsgcinlines.h"
#include "gc/ForkJoinNursery-inl.h"

check_spidermonkey_style.py reports an error for this.  The error goes away if I move the last file first.  The reason is that the is_module_header incorrectly considers the -inl.h file a module header, which it clearly is not.

Naively I would say that it's wrong for the module_name function to be stripping inlines.h and -inl.h from file names, since those will never be module header files.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Odd, I would expect that to show up elsewhere if it's broken. njn wrote the script, so let's loop him in.

Comment 2

[Lars T Hansen [:lth]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #1)
> Odd, I would expect that to show up elsewhere if it's broken. njn wrote the
> script, so let's loop him in.

I agree, it's the kind of problem you would have thought would not go unnoticed for long.

It turns out that if you start looking you will see that people have run into it but have not put up a fight: gc/Nursery.cpp includes gc/Nursery-inl.h as the first header file; ditto for vm/ErrorObject.cpp, vm/Runtime.cpp, vm/ObjectImpl.cpp (I stopped looking at this point).

IMHO an inline file should never be considered a module header, and files containing inlines should never be forced to precede non-inline header files.  The main point of separating type definitions into one file and inline implementations into another is to allow types to reference each other cyclically, and for each to have inline functions that reference the other type, and requiring an inline file to come first in large part defeats that.

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

Oh! Actually, I didn't realize that this is about a .cpp file. Back njn and I were fixing the include ordering mess, we came up with the following system:

1) .cpp module includes -inl.h/inlines.h header first if it exists, and the normal header otherwise.
2) -inl.h/inlines.h header includes normal header first.
3) Normal header has no business including any -inl.h/inlines.h headers.

So in this case what you want is

#include "gc/ForkJoinNursery-inl.h"

#include <inttypes.h>

#include "prmjtime.h"

#include "gc/Heap.h"
#include "vm/ArrayObject.h"
#include "vm/ForkJoin.h"
#include "vm/TypedArrayObject.h"

#include "jsgcinlines.h"

Comment 4

[Lars T Hansen [:lth]]

I see how that is a workaround - and as I wrote initially that's pretty much how I got it to work (without hacking the script, though I have hacked it now :-) - but again IMO it largely defeats the purpose of splitting the type definition file from the method implementation file.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

Not sure this answers your concerns, but the idea is that you have
1) Functions for which performance isn't critical. Their definitions live in a .cpp file and they're declared in a normal header for outside use.
2) Performance critical functions, which should really be inlined. These can go in an -inl.h/inlines.h header, and separate .cpp modules can include those directly. Since their .cpp module is also likely to use them, and the inlined functions are likely to depend on a subset of the headers that the .cpp module uses, we include it at the top of the .cpp file to trim down the required includes.

Classes available for outside use should have their structure documented in a header file, and headers should forward declare anything they don't strictly need to know the inside structure of (inheritance is an obvious exception to this). I don't think -inl.h/inlines.h headers are supposed to play a role for classes, but I might just be forgetting something.

Comment 6

[Lars T Hansen [:lth]]

I agree with all of that (or in large part anyhow).  The reason I'm making a fuss is that it has been my experience that interdependent classes need to export functions that are both performance-critical and dependent on other types.  Frequently enough in recursively defined systems like compilers and interpreters you end up with the following type of situation where two classes depend on each others' inline functions:

  class D;
  class C { int foo(D* x) { return a+x->bar(this); }  int baz();  int a; }

  class D { int bar(C* y) { return b+y->baz(); }  int b; }

If C and D are defined in different header files no ordering will allow the program to compile, of course.  The systematic way of structuring it is to just factor all inline functions into their own files, to be included after all defining headers:

  // C.h
  class D;
  class C { int foo(D* x);  int baz();  int a; }
  
  // C-inl.h
  int C::foo(D* x) { return a+x->bar(this); }

  // D.h
  class C;
  class D { int bar(C* y);  int b; }

  // D-inl.h
  int D::bar(C* y) { return b+y->baz(); }

  // C.cpp might look like this if baz does not need the inline methods or D:
  #include "C.h"

  int C::baz() { ... }

  // E.cpp, which is a client of both C and D, would look like this:
  #include "C.h"
  #include "D.h"

  #include "C-inl.h"
  #include "D-inl.h"

  ...

With this structure no amount of interdependence among the current or future inline functions of C and D would muck up the header file structure or order; the order is stable because type definition is separate from method definition, even for inline methods.

Anyhow it's just my opinion, I was surprised that the type definition was being conflated with the inline function definitions in the way it is.  I can let it go, but I'd be curious to hear Nick's opinion as well.

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Hmm. In this case, couldn't you include both C.h and D.h in (both) C-inl.h and D-inl.h? The header guards should ensure that they don't get pulled in multiple times - and then E.cpp could simply include C-inl.h and D-inl.h without having to bother with the normal headers. I haven't had to think about this in a while though.

Comment 8

[Nicholas Nethercote [inactive]]

IIRC, the checking script just codified the standard practice (documented at https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.23include_ordering). If you want to change the standard please email the JS list with an explanation. Having said that, the current standard isn't causing problems AFAIK.

Comment 9

[Lars T Hansen [:lth]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #7)
> Hmm. In this case, couldn't you include both C.h and D.h in (both) C-inl.h
> and D-inl.h?

That seems likely.  It strikes me as awkward - mixing concerns - but I'm inclined to believe it will work.  And then we're well into the territory of personal preference...

Comment 10

[Lars T Hansen [:lth]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> IIRC, the checking script just codified the standard practice (documented at
> https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#.
> 23include_ordering). If you want to change the standard please email the JS
> list with an explanation. Having said that, the current standard isn't
> causing problems AFAIK.

I'll back off for now.  Thanks for the link.