Closed Bug 1063233 Opened 6 years ago Closed 6 years ago

JS::ubi::Node specializations for SpiderMonkey types should live with those types' definitions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(3 files, 4 obsolete files)

JS::ubi::Node is designed to allow the code that implements the ubi::Node methods for a particular referent type live with the implementation of that referent type. This lets us keep all the code that understands a particular type in one place.

At the moment, all JS::ubi::Node support for SpiderMonkey types is in js/public/UbiNode.h and js/src/vm/UbiNode.cpp. This means that js/public/UbiNode.h includes incomplete declarations for internal SpiderMonkey types like js::Shape and js::BaseShape. This is dumb, and dumb in a way that ubi::Node was designed to avoid.
Attachment #8484623 - Flags: review?(terrence) → review+
Comment on attachment 8484625 [details] [diff] [review]
Move JS::ubi::Node support for js::Shape and js::BaseShape next to their declarations and definitions.

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

::: js/src/vm/Shape.cpp
@@ +1578,5 @@
> +template<> const jschar TracerConcrete<js::BaseShape>::concreteTypeName[] =
> +    MOZ_UTF16("js::BaseShape");
> +
> +} // namespace ubi
> +} // namespace JS

Why does this decl need to be in the namespace but the other does not? Please use the same style as JS::ubi::TracerConcreate<js::Shape> here if at all possible.
Attachment #8484625 - Flags: review?(terrence) → review+
Attachment #8484626 - Flags: review?(terrence) → review+
Attachment #8484628 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #5)
> Why does this decl need to be in the namespace but the other does not?
> Please use the same style as JS::ubi::TracerConcreate<js::Shape> here if at
> all possible.

I botched it. Thanks for catching that.
Flags: in-testsuite-
Target Milestone: --- → mozilla35
Attachment #8488278 - Flags: review?(terrence)
Flags: needinfo?(jimb)
Okay, I've got a clean (well, starred) try push for this one that is fully non-unified:
https://tbpl.mozilla.org/?tree=Try&rev=cb4409b1980a
Attachment #8484623 - Attachment is obsolete: true
Attachment #8484625 - Attachment is obsolete: true
Attachment #8484626 - Attachment is obsolete: true
Attachment #8484628 - Attachment is obsolete: true
Attachment #8488280 - Flags: review?(terrence)
Not requesting review yet; let's see what happens in bug 1066193.
Comment on attachment 8488278 [details] [diff] [review]
Provide default definitions for optional JS::ubi::Node methods in Base.

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

Okay. It's a bit worrisome that there are contexts where the actual defs aren't available, but I guess that's the cost of diaspora with C++'s build model.
Attachment #8488278 - Flags: review?(terrence) → review+
Comment on attachment 8488280 [details] [diff] [review]
Move JS::ubi::Node specializations for SpiderMonkey types closer to those types' definitions.

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

Nice!
Attachment #8488280 - Flags: review?(terrence) → review+
Thanks for the reviews!

Because we've had such problems in this bug, here's another Try push, this time without the unification-disabling patch. Gotta try everything both ways.

https://tbpl.mozilla.org/?tree=Try&rev=d70d3d0b03d2
You need to log in before you can comment on or make changes to this bug.