Closed
Bug 1130708
Opened 9 years ago
Closed 9 years ago
Remove js::types namespace
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
Attachments
(1 file)
496.26 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter 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 1•9 years ago
|
||
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+
Assignee | ||
Comment 2•9 years ago
|
||
Thanks again for the quick review! https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3bc49e9cf5
Comment 3•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ee3bc49e9cf5
Assignee: nobody → bhackett1024
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•