In ES5, every access to a property used a string, generated by ToString. ES6 makes this access-by-canonical-type concept explicit with a ToPropertyKey method that implicitly produces a property key type, used to access properties. We should have a PropertyKey concept as well, because it makes clear when a key is a PropertyName* versus when it's a uint32_t (and in the future, versus when it's an ES6 symbol, should those be implemented). This also makes a nice JSAPI improvement over the specific-to-us jsid concept. Probably we want PropertyKey and subclasses to indicate specific types -- index, name, symbol -- but I'm not sure exactly how it all should play out, so let's introduce PropertyKey and keep playing it by ear.

I'll be touching these in here, so they should be organized in a separate patch.

Obviously we can plaster whatever "these details are all private details" messages we want all over this. But having the Value class separate from the jsval layout details is just confusing.

Value depends on Anchor because there's an Anchor<Value>::~Anchor specialization, so we need js/public/Anchor.h. A little sad, since we're trying to get rid of the need for Anchor, but oh well. This makes it easier to remove Anchor eventually, so there is that small plus.

Now that Anchor's extracted, Value can be extracted to the correct location, too. All the implementation inlines could be moved into Value methods, but that seems clearly separate-patch-land. And maybe not something to do here, since it's kind of tedious and not important to this.

As discussed on IRC on Friday or so.

There's probably more that could be done here, like PropertyKey subclasses, but this is enough of a start. And I think exactly how this plays out will be influenced by what uses and users look like.

Comment on attachment 709786 [details] [diff] [review] 2 Move jsval.h to js/public/Value.h Review of attachment 709786 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/methodjit/PunboxAssembler.h @@ +11,4 @@ > #include "assembler/assembler/MacroAssembler.h" > #include "methodjit/MachineRegs.h" > #include "methodjit/RematInfo.h" > +#include "js/Value.h" Doesn't j go before m?

Comment on attachment 709785 [details] [diff] [review] 1 - Organize the #includes at the start of jsapi.h, for sanity Review of attachment 709785 [details] [diff] [review]: ----------------------------------------------------------------- Feel free to lose the comment on <limits> if you want.

Comment on attachment 709791 [details] [diff] [review] 4 - Move Value into js/public/Value.h Review of attachment 709791 [details] [diff] [review]: ----------------------------------------------------------------- r=me with this comment tweaked: >+ // Value must be POD so that MSVC will pass it by value and not by memory >+ // (bug 689101); the same is true for SPARC as well (bug 737344). "by memory"? The old comment ("in memory") was better; bug 689101 comment 26 explains that the reason is really "so that MSVC does not compile Value return values as out params". > static MOZ_ALWAYS_INLINE jsval > OBJECT_TO_JSVAL(JSObject *obj) > { > if (obj) > return IMPL_TO_JSVAL(OBJECT_TO_JSVAL_IMPL(obj)); >- return JSVAL_NULL; >+ return IMPL_TO_JSVAL(BUILD_JSVAL(JSVAL_TAG_NULL, 0)); > } Good change. ::: js/public/Value.h @@ +13,5 @@ > +#include "mozilla/Attributes.h" > +#include "mozilla/FloatingPoint.h" > +#include "mozilla/Likely.h" > + > +#include <limits> /* for std::numeric_limits */ (...Or I guess this would be the place to feel free to delete the comment, now.)

Comment on attachment 709793 [details] [diff] [review] 6 - Add a js/public/PropertyKey.h and js/src/vm/PropertyKey.cpp Review of attachment 709793 [details] [diff] [review]: ----------------------------------------------------------------- OK, cool. ::: js/public/PropertyKey.h @@ +38,5 @@ > +class PropertyKey > +{ > +#if defined(_MSC_VER) || defined(__sparc) > + // PropertyKey must be POD for MSVC/SPARC ABI reasons; see the comment in > + // Value.h about Value being POD for details. Hmm. I don't think it's strictly necessary. It would matter if an API entry point that returns PropertyKey is hot... which should be effectively never, right? So I'd try dropping this. @@ +45,5 @@ > + Value v; > + friend bool detail::ToPropertyKeySlow(JSContext *cx, HandleValue v, PropertyKey *key); > + > + public: > + PropertyKey(uint32_t index) : v(PrivateUint32Value(index)) {} "explicit" please, so that mistakes like passing an int (or NULL) to a function expecting a PropertyKey are detected at compile time (rather than implicitly invoking this constructor). @@ +50,5 @@ > + > + bool isIndex(uint32_t *index) { > + if (!v.isInt32()) > + return false; > + *index = v.toPrivateUint32(); Heh! I'd be happier with just casting between int32_t and uint32_t in these inline methods, and not using private anything; I thought the intent with the private stuff was that it wasn't supposed to be queryable in this way. Certainly the code here *looks* wrong. Your call!

I changed the by-memory comment, dropped PODness on PropertyKey for now, added explicit, and added comments by the various isFoo methods in PropertyKey. Regarding private-value stuff being queryable, it's not -- this, as implementation, is relying on implementation details for queryability. I added a comment telling embedders not to rely on this. https://hg.mozilla.org/integration/mozilla-inbound/rev/fbc1e196ca87 https://hg.mozilla.org/integration/mozilla-inbound/rev/dff079686e0b https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1ecab23f6f https://hg.mozilla.org/integration/mozilla-inbound/rev/f13e3cac0c1b https://hg.mozilla.org/integration/mozilla-inbound/rev/06fe741ec953 https://hg.mozilla.org/integration/mozilla-inbound/rev/43a5c9890c22

Sigh, too SpiderMonkey-centric. Then again I found a few more jsval.h references even in js/src, so not all bad. https://hg.mozilla.org/integration/mozilla-inbound/rev/7a9887e1f55e

https://hg.mozilla.org/mozilla-central/rev/fbc1e196ca87 https://hg.mozilla.org/mozilla-central/rev/dff079686e0b https://hg.mozilla.org/mozilla-central/rev/7f1ecab23f6f https://hg.mozilla.org/mozilla-central/rev/f13e3cac0c1b https://hg.mozilla.org/mozilla-central/rev/06fe741ec953 https://hg.mozilla.org/mozilla-central/rev/43a5c9890c22 https://hg.mozilla.org/mozilla-central/rev/7a9887e1f55e