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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(7 attachments)

3.01 KB, patch
jandem
: review+
Details | Diff | Splinter Review
594 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
589 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
93.04 KB, patch
jandem
: review+
Details | Diff | Splinter Review
688 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
631 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
48.40 KB, patch
jandem
: review+
Details | Diff | Splinter Review
Started doing these debugging what turned out to be an unrelated issue.  They're good hygiene, tho, so worth doing regardless.
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.
Attachment #9009517 - Flags: review?(jdemooij)
I can't find a thing in UnboxedObject.h that needed this.
Attachment #9009518 - Flags: review?(jdemooij)
Nor this.
Attachment #9009519 - Flags: review?(jdemooij)
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.
Attachment #9009520 - Flags: review?(jdemooij)
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.)
Attachment #9009521 - Flags: review?(jdemooij)
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.
Attachment #9009523 - Flags: review?(jdemooij)
Attachment #9009517 - Flags: review?(jdemooij) → review+
Attachment #9009518 - Flags: review?(jdemooij) → review+
Attachment #9009519 - Flags: review?(jdemooij) → review+
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.
Attachment #9009520 - Flags: review?(jdemooij) → review+
Attachment #9009521 - Flags: review?(jdemooij) → review+
Attachment #9009522 - Flags: review?(jdemooij) → review+
Attachment #9009523 - Flags: review?(jdemooij) → review+
Pushed by jwalden@mit.edu:
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
You need to log in before you can comment on or make changes to this bug.