chakrashim: Enable new Get/Has/Set etc. Property from String #422
Conversation
| @@ -66,16 +66,11 @@ bool Object::Set(Handle<Value> key, Handle<Value> value) { | |||
|
|
|||
| Maybe<bool> Object::Set(Handle<Value> key, Handle<Value> value, | |||
| PropertyAttribute attribs, bool force) { | |||
| JsPropertyIdRef idRef; | |||
|
|
|||
| if (jsrt::GetPropertyIdFromValue((JsValueRef)*key, &idRef) != JsNoError) { | |||
There was a problem hiding this comment.
Previously this code path worked for Symbols as well as strings; do you know if that is ever used?
There was a problem hiding this comment.
Actually, it also tried to work for all values, and would stringify things that weren't strings or symbols.
There was a problem hiding this comment.
consider symbol is there. I'm not sure if we should! support other types though.
| @@ -66,16 +66,11 @@ bool Object::Set(Handle<Value> key, Handle<Value> value) { | |||
|
|
|||
| Maybe<bool> Object::Set(Handle<Value> key, Handle<Value> value, | |||
| PropertyAttribute attribs, bool force) { | |||
| JsPropertyIdRef idRef; | |||
There was a problem hiding this comment.
Since this idRef is used twice, is the overhead of invoking GetOrAddPropertyRecord twice (once per JsObject*Property method) smaller than the additional JSRT calls involved in fetching the id ref up front?
There was a problem hiding this comment.
no, not if it's literal or prop string. considering createString will create literalstring and presumably user will call set via a string variable that was also created on native land...
| bool result = false; | ||
| if (JsDefineProperty(object, propertyIdRef, descriptor, &result) | ||
| if (JsObjectDefineProperty(object, prop, descriptor, &result) |
There was a problem hiding this comment.
I believe that this isn't quite right; Right now this code path isn't going to be used until we fix our handling of vm.runInContext, but when it is used it is perfectly allowable for prop to be a Symbol, which would not be handled with this change.
There was a problem hiding this comment.
Right, Symbol support should be in master no time.
Update chakrasim to enable chakra-core/ChakraCore#3875