JSAtomList should be a templated type to allow strict type checking

RESOLVED DUPLICATE of bug 649576

Status

()

Core
JavaScript Engine
--
minor
RESOLVED DUPLICATE of bug 649576
9 years ago
6 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 3

8 years ago
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.
Attachment #403864 - Flags: review?(igor)
(Assignee)

Comment 4

8 years ago
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>
Attachment #399535 - Attachment is obsolete: true
Attachment #403866 - Flags: review?(igor)
(Assignee)

Comment 5

8 years ago
This patch series causes no js test suite regressions, and has no effect on Sunspider scores.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

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

Obsoleted by Chris Leary's work.
Attachment #403864 - Flags: review?(igor)
(Assignee)

Comment 7

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

Obsoleted by Chris Leary's work.
Attachment #403866 - Flags: review?(igor)
(Assignee)

Comment 8

6 years ago
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.