Closed Bug 849273 Opened 7 years ago Closed 7 years ago

Investigate splitting the js and JS namespaces

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file, 4 obsolete files)

At the moment, the JS namespace is included in the js namespace by a using declaration in js/public/Utility.h.

For bug 781070 we want to have separate js::NullPtr and JS::NullPtr classes, so this bug is to investigate removing the inclusion.
Splitting the namespaces sounds fine to me on its face and it would be more consistent with what we do for namespace mozilla.  I see two (solvable) problems though:
 - internal headers would have to explicitly qualify super-common names like Value
 - we couldn't put 'using namespace JS;' at the top of our .cpps, like we do with 'js' and 'mozilla', since this would cause a naming ambiguity between js::NullPtr and JS::NullPtr.  That means *all* code would have to explicitly qualify names like Value.

There are a couple of options, but the best I can see atm is to:
 1. never have 'using namespace JS;' anywhere
 2. in some central *internal* header we explicitly import all the JS:: names that we want to make unqualified reference to via 'using JS::Value;'.

IIUC, that'll give us short names internally and no ambiguity for NullPtr.
This adds JS:: prefixes where necessary in header files, and adds "using namespace JS" in .cpp files.
(In reply to Luke Wagner [:luke] from comment #1)

Oh hey, I just posted my work in progress patches for this before seeing your comment.  I prefer the idea of having a bunch of names we import into js::, that would be much less cumbersome.
(In reply to Jon Coppeard (:jonco) from comment #6)
Great!  IIUC, if we import all the non-ambiguous names, the only js::/JS:: qualification we'd have to add would be for NullPtr in headers.
Simpler approach as per Luke's suggestion.

This splits the namespaces but explicitly imports frequently used names back into js::.  jsapi.h turned out to be the most convenient place to do this, since it needs to happen after declaration but before use.
Attachment #722849 - Attachment is obsolete: true
Attachment #722851 - Attachment is obsolete: true
Attachment #722852 - Attachment is obsolete: true
Attachment #722854 - Attachment is obsolete: true
Attachment #726258 - Flags: review?(terrence)
Comment on attachment 726258 [details] [diff] [review]
Split namespaces but import frequently used JS:: names into js::

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

Excellent. The external changes appear to be actual fixes for js:: usage that should never have slipped outside the engine. The internal changes also seem reasonable.

::: js/public/Utility.h
@@ +30,5 @@
>  /* The mozilla-shared reusable template/utility namespace. */
>  namespace mozilla {}
>  
>  /* The private JS engine namespace. */
> +namespace js {}

\o/

::: js/src/jsapi.cpp
@@ +6261,5 @@
>      AssertHeapIsIdle(cx);
>      CHECK_REQUEST(cx);
>  
>      RootedValue reviver(cx, reviverArg), value(cx);
> +    if (!ParseJSONWithReviver(cx, JS::StableCharPtr(chars, len), len, reviver, &value))

StableCharPtr and RootedValue are both imported into js:: in jsapi.h. Why are the StableCharPtr changes required here?
Attachment #726258 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #9)
> Comment on attachment 726258 [details] [diff] [review]

> StableCharPtr and RootedValue are both imported into js:: in jsapi.h. Why
> are the StableCharPtr changes required here?

They're not.  Fixed.
https://hg.mozilla.org/mozilla-central/rev/fa78767a3e78
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.