12.53 KB, patch
|Details | Diff | Splinter Review|
63.69 KB, patch
|Details | Diff | Splinter Review|
At the moment, SpiderMonkey uses the JSAtomList (or JSAtomSet) type to map atoms to JSDefinition *, jsatomid, uint32, and jsval values. This code depends on JSAtomList clients using accessor macros consistently; failure to do so is a run-time error. JSAtomList (and JSAtomListElement) should be templated types, parameterized on the value type: JSAtomList<JSDefinition *>, etc.
Created attachment 399535 [details] [diff] [review] Make JSAtomList a templated type, to enable strict typing. This is the basic idea, but it introduces a lot of regressions at the moment: js1_5/Scope/regress-77578-001.js js1_5/Scope/scope-003.js js1_5/Scope/regress-220362.js js1_5/Scope/regress-181834.js js1_5/Scope/regress-446026-01.js js1_5/GC/regress-383269-01.js js1_5/GC/regress-383269-02.js js1_5/Object/regress-137000.js js1_5/decompilation/regress-352013.js js1_5/Regress/regress-169559.js js1_5/Regress/regress-451884.js js1_5/Regress/regress-460024.js js1_5/Regress/regress-455775.js js1_5/Regress/regress-470758-02.js js1_5/Regress/regress-127557.js js1_5/Regress/regress-452008.js js1_5/Regress/regress-504078.js js1_5/Regress/regress-482421.js js1_5/Regress/regress-288688.js js1_5/Regress/regress-240577.js js1_5/Regress/regress-459085.js js1_5/Regress/regress-383674.js js1_5/Regress/regress-456494.js js1_5/Regress/regress-224956.js js1_5/Regress/regress-449666.js js1_5/Regress/regress-480244.js js1_5/Regress/regress-477758.js js1_5/Regress/regress-398609.js
Assignee: general → jim
Summary: AtomList should be a templated type to allow strict type checking → JSAtomList should be a templated type to allow strict type checking
Old and tricky code. Thanks for cleaning up. Beware list -> hashtable on growth past threshold abstraction-breaking. /be
Created attachment 403864 [details] [diff] [review] Be more precise about JSDefinition/JSParseNode type usage. Before we can make JSAtomMap strictly typed, we need to tighten up the types used for the nodes we add to lexdeps and decls. These ought to be JSAtomMap<JSDefinition *>, but in many cases the values we store in them are JSParseNode *, and we rely on the intermediate conversion to void * to make the code palatable to the compiler. NewDefinitionNode provides a typed interface for creating JSDefinitions. JSParseNode::becomeDefinition provides a typed interface for promotion. JSParseNode::asDefinition provides a typed, checked interface for downcasting. I apologize for all the unpleasant-to-review renamings. Using the correct types for nodes entails changing some JSParseNode * variables to JSDefinition *; I wanted to preserve the rule that JSDefinition * values have names based on 'dn'; and that entailed renaming some clearly secondary nodes already named 'dn' to something more descriptive.
Created attachment 403866 [details] [diff] [review] Make JSAtomList a templated type, to enable strict typing. To reduce code duplication, we have loosely-typed base classes: JSAtomListBase JSAtomListElementBase JSAtomListIteratorBase And then we defined strictly-typed templates that inherit from the loosely-typed base classes, and define only inline member functions: JSAtomList<T> JSAtomListElement<T> JSAtomListIterator<T>
This patch series causes no js test suite regressions, and has no effect on Sunspider scores.
Status: NEW → ASSIGNED
Comment on attachment 403864 [details] [diff] [review] Be more precise about JSDefinition/JSParseNode type usage. Obsoleted by Chris Leary's work.
Comment on attachment 403866 [details] [diff] [review] Make JSAtomList a templated type, to enable strict typing. Obsoleted by Chris Leary's work.
Chris Leary took care of all of this in bug 649576.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 649576
You need to log in before you can comment on or make changes to this bug.