Closed Bug 1130708 Opened 9 years ago Closed 9 years ago

Remove js::types namespace

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
A lot of places use the js::types namespace, and since with bug 1129226 most of the things in this namespace have pretty type specific names (TypeSet, TypeNewScript, ...) there doesn't seem to be much value in cluttering up code with types:: prefixes.

The attached patch removes this namespace, and either renames or encapsulates classes and function names in the namespace that had pretty generic names, mainly moving js::types::Type into js::TypeSet.
Attachment #8560850 - Flags: review?(jdemooij)
Comment on attachment 8560850 [details] [diff] [review]
patch

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

Nice.

::: js/src/asmjs/AsmJSModule.cpp
@@ +534,5 @@
>      // Note that the TypeScript is never discarded while the script has a
>      // BaselineScript, so if those checks hold now they must hold at least until
>      // the BaselineScript is discarded and when that happens the FFI exit is
>      // patched back.
> +    if (!TypeScript::ThisTypes(script)->hasType(TypeSet::UndefinedType()))

Not for this patch, but I think we should rename TypeScript to ObservedTypes or something, since that's all it is nowadays. That also avoids confusion with TypeScript-the-language.

::: js/src/gc/Marking.cpp
@@ +1458,1 @@
>          if (prop)

Nit: while you're here:

if (ObjectGroupProperty *prop = group->getProperty(i))

::: js/src/jit/IonCaches.cpp
@@ +1268,5 @@
>      CacheLocation *locs = ion->getCacheLocs(locationIndex);
>      for (size_t i = 0; i < numLocations; i++) {
>          CacheLocation &curLoc = locs[i];
> +        StackTypeSet *bcTypes =
> +            TypeScript::BytecodeTypes(curLoc.script, curLoc.pc);

This should fit on one line now I think.

::: js/src/jit/MIR.cpp
@@ +4492,1 @@
>          if (key) {

Nit: if (TypeSetObjectKey *key = types->getObject(i)) {

@@ +4562,1 @@
>          if (key) {

And here.

@@ +4614,2 @@
>              if (key)
> +                observed->addType(TypeSet::ObjectType(key), alloc);

And here.

::: js/src/jsinfer.cpp
@@ +200,5 @@
>      return colors[DefaultHasher<TypeSet *>::hash(types) % 7];
>  }
>  
>  const char *
> +js::TypeString(TypeSet::Type type)

Do you think it'd make sense to make these functions static methods of TypeSet? Or maybe a non-static method of Type?

::: js/src/jsinfer.h
@@ +814,5 @@
>   * We establish these by using write barriers on calls to setProperty and
>   * defineProperty which are on native properties, and on any jitcode which
>   * might update the property with a new type.
>   */
> +class ObjectGroupProperty

Could we move this class into ObjectGroup, ObjectGroup::Property? Also maybe TypeSet::ObjectKey?

::: js/src/vm/ObjectGroup.h
@@ +578,5 @@
>  
>    private:
>      static ObjectGroup *defaultNewGroup(JSContext *cx, JSProtoKey key);
> +    static void setGroupToHomogenousArray(ExclusiveContext *cx, JSObject *obj,
> +                                          /* TypeSet::Type */ uintptr_t type);

Is this to avoid a cyclic dependency? It seems like Type only needs a forward-declaration of ObjectGroup, so maybe we could move TypeSet to its own header and include it here?
Attachment #8560850 - Flags: review?(jdemooij) → review+
Blocks: 1130849
https://hg.mozilla.org/mozilla-central/rev/ee3bc49e9cf5
Assignee: nobody → bhackett1024
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: