Repository navigation
Update PropertyRecord interface to preserve the record life time - #4580
Conversation
| { | ||
| this->propertyRecord = propStr->GetPropertyRecord(); | ||
| Js::PropertyRecord const * localPropertyRecord; | ||
| propStr->GetPropertyRecord(&localPropertyRecord); |
There was a problem hiding this comment.
Why the local variable here, rather than passing in &(this->propertyRecord)?
There was a problem hiding this comment.
That wouldn't trigger SWB operator
There was a problem hiding this comment.
Ah I see, makes sense. Could you please add a comment there so it doesn't get "cleaned up" later?
jackhorton
left a comment
There was a problem hiding this comment.
There is a lot of indirection that got removed in this PR. Is there any perf gain that you're aware of/expecting?
|
|
||
| public: | ||
| virtual Js::PropertyRecord const * GetPropertyRecord(bool dontLookupFromDictionary = false) override; | ||
| virtual void GetPropertyRecord(_Out_ PropertyRecord const** propRecord, bool dontLookupFromDictionary = false) override |
There was a problem hiding this comment.
Any reason why this moved to the header?
| *propRecord = this->propertyRecord; | ||
| } | ||
|
|
||
| virtual Js::PropertyId GetPropertyId() override |
There was a problem hiding this comment.
Is this override needed if you've got the same code on Js::JavascriptString?
|
|
||
| return propertyRecord; | ||
| Assert(propertyRecord != nullptr); | ||
| return propertyRecord->GetPropertyId(); |
There was a problem hiding this comment.
The propertyRecord could still be collected after this GetPropertyId() and propertyId becomes invalid, right?
There was a problem hiding this comment.
For PropertyString?
4f609f4 to
18f88ca
Compare
|
@jianchun please review. |
| Js::PropertyRecord const * localPropertyRecord; | ||
| key->GetPropertyRecord(&localPropertyRecord); | ||
| // WARNING: This will return false for PropertyStrings that are actually InternalPropertyIds | ||
| Assert(!PropertyString::Is(key) || !IsInternalPropertyId(((PropertyString*)key)->GetPropertyRecord()->GetPropertyId())); |
There was a problem hiding this comment.
!IsInternalPropertyId(((PropertyString*)key)->GetPropertyRecord()->GetPropertyId()) [](start = 43, length = 83)
nit: Since in this || branch key is PropertyString, can be simplified to !IsInternalPropertyId(((PropertyString*)key)->GetPropertyId()), keeping original one line ASSERT.
| if (propStr != nullptr) | ||
| { | ||
| this->propertyRecord = propStr->GetPropertyRecord(); | ||
| // why use local variable ? read SWB wiki |
There was a problem hiding this comment.
nit: This comment isn't really helpful. No comment is ok -- any reader interested or doubtful could easily find out there is a type mismatch between "this->propertyRecord" vs. "->GetPropertyRecord(...).
|
@MSLaguana @jackhorton @jianchun Thanks for the review! |
…the record life time Merge pull request #4580 from obastemur:props_swb
… preserve the record life time Merge pull request #4580 from obastemur:props_swb
… interface to preserve the record life time Merge pull request #4580 from obastemur:props_swb
No description provided.