Last Comment Bug 678074 - Implement js::PropertyName to represent property names which are never store characters for an index
: Implement js::PropertyName to represent property names which are never store ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-10 15:29 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-08-14 05:00 PDT (History)
1 user (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (11.09 KB, patch)
2011-08-10 15:29 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-10 15:29:12 PDT
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.
Comment 1 Luke Wagner [:luke] 2011-08-10 16:52:36 PDT
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?
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-12 12:48:23 PDT
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.
Comment 3 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-14 05:00:29 PDT
http://hg.mozilla.org/mozilla-central/rev/137325e8319c

Note You need to log in before you can comment on or make changes to this bug.