515441 - JSAtomList should be a templated type to allow strict type checking

Status: RESOLVED DUPLICATE of bug 649576

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

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.

Comment 1

[Jim Blandy :jimb]

Attachment: 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

Comment 2

[Brendan Eich [:brendan]]

Old and tricky code. Thanks for cleaning up. Beware list -> hashtable on growth past threshold abstraction-breaking.

/be

Comment 3

[Jim Blandy :jimb]

Attachment: 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.

Comment 4

[Jim Blandy :jimb]

Attachment: 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>

Comment 5

[Jim Blandy :jimb]

This patch series causes no js test suite regressions, and has no effect on Sunspider scores.

Comment 6

[Jim Blandy :jimb]

Comment on attachment 403864 [details] [diff] [review]
Be more precise about JSDefinition/JSParseNode type usage.

Obsoleted by Chris Leary's work.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 403866 [details] [diff] [review]
Make JSAtomList a templated type, to enable strict typing.

Obsoleted by Chris Leary's work.

Comment 8

[Jim Blandy :jimb]

Chris Leary took care of all of this in bug 649576.