1491736 - Various header-organization issues related to monster #includes, bootlegged dependencies, called-before-defined, &c.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jeff Walden [:Waldo]]

Started doing these debugging what turned out to be an unrelated issue.  They're good hygiene, tho, so worth doing regardless.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Remove an unnecessary #include from "jit/IonCode.h"

IonCompilationId makes more sense in IonTypes.h, for one.  For two, vm/TypeInference.h is a gonzo-sized header, and it bootlegs some of its dependencies, and fixing that pretty much *requires* that this move to a lower-level, finer-grained header.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Remove an unnecessary #include from "vm/UnboxedObject.h"

I can't find a thing in UnboxedObject.h that needed this.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Remove an unnecessary #include from "vm/NativeObject.h"

Nor this.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Split out type-set classes and related data types from vm/TypeInference.h into vm/TypeSet.h, and move TypeNewScript from vm/TypeInference.h to vm/TypeInference-inl.h, so code can use TypeSet types without needing JSFunction, Shape, and other super-complex

Prior to this patch and prior patches, a standalone |#include "vm/JSFunction.h"| and a whole mess of headers would just not compile, because TypeNewScript's definition demanded JSFunction, Shape, &c. be defined.

The right fix seems to be to put *TypeSet classes that many headers need in their own header, move TypeNewScript to a *-inl.h so type inference code *mostly* doesn't have to see JSFunction, Shape, &c., define ~UnboxedLayout (that needs to see TypeNewScript's definition) just after it, and ObjectGroup::hasUnanalyzedPreliminaryObjects just after that for needing TypeNewScript::analyzed.

TypeNewScriptInitializer is a bit of a hack name.  If you've got a better one, say it.  But that part *is* needed by headers that don't need TypeNewScript or any of the zillion things TypeInference-inl.h pulls in, so it has to get pulled out of TypeNewScript.

UnboxedObject-inl.h needs AutoSweepObjectGroup from TypeInference.h.  I guess it got it before because ArrayObject-inl.h just happened to provide it.  Bleh.

I'm not sure I'm happy about gc/Zone.h demanding full TypeInference.h, but as long as it needs TypeZone, and that type isn't in its own header, nothing to do about it.  Maybe something to do soon.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Add an #include "vm/Shape.h" to vm/TypeInference.h so that header can compile without needing to bootleg anything

This is needed for PreliminaryObjectArrayWithTemplate::shape_ which is HeapPtr<Shape*>.  (In case you've forgotten, for HeapPtr<T*>, T::something gets used when instantiating it, so you need T's definition.)

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Make sure JSFunction-inl.h #includes the definition of NativeObject::dynamicSlotsCount that it calls

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Move the various meta-object operations into a new vm/ObjectOperations-inl.h header rather than sharding declaration and definition across separate headers, thereby risking used-before-definition problems

The current setup is pretty messy, and it piles on to two headers that mostly ought be dedicated to the narrow structures they care about.  This unites all the concepts in one sensible spot.

Some of the more exotic symbol stuffs debatably ought not be elevated into the concept these headers implement, but I'm not sure they definitely belong somewhere else like SymbolType.h or whatever, and it's not worth losing sleep over.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 9009520 [details] [diff] [review]
Split out type-set classes and related data types from vm/TypeInference.h into vm/TypeSet.h, and move TypeNewScript from vm/TypeInference.h to vm/TypeInference-inl.h, so code can use TypeSet types without needing JSFunction, Shape, and other super-complex

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

More of a rubberstamp because these |hg cp| patches are hard to review, but looks good.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9300e88eb74
Remove an unnecessary #include from "jit/IonCode.h".  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0ae19801fc
Remove an unnecessary #include from "vm/UnboxedObject.h".  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/efcfd2f95057
Remove an unnecessary #include from "vm/NativeObject.h".  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/60df00079cd4
Split out type-set classes and related data types from vm/TypeInference.h into vm/TypeSet.h, and move TypeNewScript from vm/TypeInference.h to vm/TypeInference-inl.h, so code can use TypeSet types without needing JSFunction, Shape, and other super-complex types as well (via HeapPtr<T*> fields in TypeNewScript).  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/740b790557b9
Add an #include "vm/Shape.h" to vm/TypeInference.h so that header can compile without needing to bootleg anything.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/47d60f29b7cf
Make sure JSFunction-inl.h #includes the definition of NativeObject::dynamicSlotsCount that it calls.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef024a29617
Move the various meta-object operations into a new vm/ObjectOperations-inl.h header rather than sharding declaration and definition across separate headers, thereby risking used-before-definition problems.  r=jandem

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d9300e88eb74
https://hg.mozilla.org/mozilla-central/rev/dc0ae19801fc
https://hg.mozilla.org/mozilla-central/rev/efcfd2f95057
https://hg.mozilla.org/mozilla-central/rev/60df00079cd4
https://hg.mozilla.org/mozilla-central/rev/740b790557b9
https://hg.mozilla.org/mozilla-central/rev/47d60f29b7cf
https://hg.mozilla.org/mozilla-central/rev/3ef024a29617