chakrashim: Enable new Get/Has/Set etc. Property from String#422
chakrashim: Enable new Get/Has/Set etc. Property from String#422obastemur wants to merge 1 commit intonodejs:masterfrom
Conversation
| 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.
|
|
||
| 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.
2 participants
Update chakrasim to enable chakra-core/ChakraCore#3875