Implement js::PropertyName to represent property names which are never store characters for an index

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla8
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 552239 [details] [diff] [review]
Patch

PropertyName will eventually replace jsid internally, so that no one internally need ever consider (except when fed an unknown value for a property) whether to look in element storage or property storage for an object property.  Externally, jsid will stick around for simplicity, probably for some time.
Attachment #552239 - Flags: review?(luke)

Comment 1

6 years ago
Comment on attachment 552239 [details] [diff] [review]
Patch

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

::: js/src/jsapi-tests/testPropertyName.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Have you considered merging this into that other jsapi-test .cpp you added recently?  Its nice to sorta minimize the number or jsapi-test .cpps (not tests!) since I have to build them all the time when I make a shell.

::: js/src/vm/String.cpp
@@ +356,5 @@
> +
> +    if (cp != end)
> +        return false;
> +
> +    if (oldIndex < UINT32_MAX / 10 || (oldIndex == UINT32_MAX / 10 && c <= (UINT32_MAX % 10))) {

Comment to the effect of "strlen("9999999999") == UINT32_CHAR_BUFFER_LENGTH, but greater than UINT32_MAX."

::: js/src/vm/String.h
@@ +814,5 @@
>      return &asFixed();
>  }
>  
> +inline js::PropertyName *
> +JSAtom::asPropertyName()

Can you put this in String-inl.h?  The few inlines in String.h are ones that are used super-commonly where its nice to avoid requiring String-inl.h.  Or do you forsee this being used everywhere?
Attachment #552239 - Flags: review?(luke) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/137325e8319c

(In reply to Luke Wagner [:luke] from comment #1)
> Have you considered merging this into that other jsapi-test .cpp you added
> recently?  Its nice to sorta minimize the number or jsapi-test .cpps (not
> tests!) since I have to build them all the time when I make a shell.

Done.  I generally haven't thought about keeping the number minimal -- I think that's a good idea for shell tests, to minimize interdependencies -- but I guess extra compile time is more notable than extra shell startup/shutdown time.

> > +inline js::PropertyName *
> > +JSAtom::asPropertyName()
> 
> Can you put this in String-inl.h?  The few inlines in String.h are ones that
> are used super-commonly where its nice to avoid requiring String-inl.h.  Or
> do you forsee this being used everywhere?

No idea.  I can put it in the inlines file, I've just always thought of them as the "put it here because it has cyclic dependencies" workaround, and not as a good place to put things as a general matter of course.
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/137325e8319c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.