INSTALLED_HEADERS and you

13 views
Skip to first unread message

Jeff Walden

unread,
Jun 16, 2011, 5:15:57 PM6/16/11
to JS Internals list
Back a few weeks ago or so I added jsnum.h to the INSTALLED_HEADERS list. It turned out that header had been removed from that list, deliberately, so that Gecko couldn't use it. (Such dependencies slow down the build quite a bit for purely-JS-internal changes.)

Some headers clearly must be in that list -- jsapi.h, jsdbgapi.h, jspubtd.h, for example. Some headers have no reason to be in that list, and I just removed jsnum.h (again) and a dozen or so other such headers from the list, with a slightly-greater-than-minor amount of effort. Then there's a bulk which shouldn't be there, but for XPConnect and similar incest. It'll take awhile to work through those, but if we make a little effort here, a little effort there, consistently, I think we can clear it out. In the meantime:

Don't add new headers to INSTALLED_HEADERS!

There will probably be exceptions. But often there's a better approach -- say, moving a method definition to an inlines.h. And it's hard to claw these things back from Gecko (I'm looking at you, jsobj.h/jsscope.h/jscntxt.h). So if you want to add a header, you should get positive feedback from multiple peers before doing so.

Jeff

Brendan Eich

unread,
Jun 16, 2011, 6:39:13 PM6/16/11
to Jeff Walden, JS Internals list
On Jun 16, 2011, at 2:15 PM, Jeff Walden wrote:

> Back a few weeks ago or so I added jsnum.h to the INSTALLED_HEADERS list. It turned out that header had been removed from that list, deliberately, so that Gecko couldn't use it. (Such dependencies slow down the build quite a bit for purely-JS-internal changes.)
>
> Some headers clearly must be in that list -- jsapi.h, jsdbgapi.h, jspubtd.h, for example. Some headers have no reason to be in that list, and I just removed jsnum.h (again) and a dozen or so other such headers from the list, with a slightly-greater-than-minor amount of effort. Then there's a bulk which shouldn't be there, but for XPConnect and similar incest. It'll take awhile to work through those, but if we make a little effort here, a little effort there, consistently, I think we can clear it out. In the meantime:
>
> Don't add new headers to INSTALLED_HEADERS!

Thanks for doing this -- it is a good fight we should fight.


> There will probably be exceptions. But often there's a better approach -- say, moving a method definition to an inlines.h. And it's hard to claw these things back from Gecko (I'm looking at you, jsobj.h/jsscope.h/jscntxt.h).

Yet all three of those have js*inlines.h counterparts. Maybe fixing these bad boys is just a little more little more effort?


> So if you want to add a header, you should get positive feedback from multiple peers before doing so.

+1

/be

Jeff Walden

unread,
Jun 16, 2011, 7:46:51 PM6/16/11
to Brendan Eich, JS Internals list
On 06/16/2011 03:39 PM, Brendan Eich wrote:
>> There will probably be exceptions. But often there's a better approach -- say, moving a method definition to an inlines.h. And it's hard to claw these things back from Gecko (I'm looking at you, jsobj.h/jsscope.h/jscntxt.h).
>
> Yet all three of those have js*inlines.h counterparts. Maybe fixing these bad boys is just a little more little more effort?

Sometimes, plausibly, but generally not (although I admit to not skimming the full scope of tainting for every header I didn't remove). They want stuff like JSObject::setSlot and similar. My case was just that I added a method to jsnum.h, then included that in another header rather than put the inline in inlines.h, then use the inlines.h from the single cpp file that wanted it. That was easy to fix. Alas, wanting JSObject::setSlot, and other similar guts-ful things like that, is not.

Jeff

Luke Wagner

unread,
Jun 16, 2011, 8:16:06 PM6/16/11
to dev-tech-js-en...@lists.mozilla.org
Agreed. A few times I've tangled a bit with the #include clique and
I've wondered if there is a tool for automatically generating diagrams
(say with graphviz) of #includes. It might be nice to have this so we
could make some attempt at stratification.

> Back a few weeks ago or so I added jsnum.h to the INSTALLED_HEADERS
> list. It turned out that header had been removed from that list,
> deliberately, so that Gecko couldn't use it. (Such dependencies slow
> down the build quite a bit for purely-JS-internal changes.)
>
> Some headers clearly must be in that list -- jsapi.h, jsdbgapi.h,
> jspubtd.h, for example. Some headers have no reason to be in that
> list, and I just removed jsnum.h (again) and a dozen or so other such
> headers from the list, with a slightly-greater-than-minor amount of
> effort. Then there's a bulk which shouldn't be there, but for
> XPConnect and similar incest. It'll take awhile to work through
> those, but if we make a little effort here, a little effort there,
> consistently, I think we can clear it out. In the meantime:
>
> Don't add new headers to INSTALLED_HEADERS!
>

> There will probably be exceptions. But often there's a better
> approach -- say, moving a method definition to an inlines.h. And it's
> hard to claw these things back from Gecko (I'm looking at you,

> jsobj.h/jsscope.h/jscntxt.h). So if you want to add a header, you

> should get positive feedback from multiple peers before doing so.
>

> Jeff
> _______________________________________________
> dev-tech-js-engine-internals mailing list
> dev-tech-js-en...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals

Nicholas Nethercote

unread,
Jun 16, 2011, 8:21:20 PM6/16/11
to Luke Wagner, dev-tech-js-en...@lists.mozilla.org
On Fri, Jun 17, 2011 at 10:16 AM, Luke Wagner <lu...@mozilla.com> wrote:
> Agreed.  A few times I've tangled a bit with the #include clique and I've
> wondered if there is a tool for automatically generating diagrams (say with
> graphviz) of #includes.  It might be nice to have this so we could make some
> attempt at stratification.

https://bugzilla.mozilla.org/show_bug.cgi?id=634839 might be of
interest. There's a Dutch CS student who dived into it with great
gusto. He's been quiet lately because he has exams, but I'm hoping
he'll get back into it soon.

Nick

Reply all
Reply to author
Forward
0 new messages