Cleanup JavascriptEnumerator interface #1400
Conversation
The JavascriptEnumerator function GetCurrentAndMoveNext is misleading because. Because it move to the next one before return the next element that we moved to. Change to MoveAndGetNext. Also remove MoveNext/GetCurrentIndex/GetCurrentPropertyId, which is equivalent to GetCurrentAndMoveNext. Move all usage of those API to MoveAndGetNext.
| @@ -155,11 +150,11 @@ namespace Js | |||
| BOOL ForInObjectEnumerator::MoveNext() | |||
There was a problem hiding this comment.
are we going to change this sometime as well? #Resolved
There was a problem hiding this comment.
This is still used in Chakra.dll #Resolved
I'm always a bit confused here: should propertyId & propertyName be consistent at this point? should we assert at this point? #Resolved Refers to: lib/Runtime/Library/JavascriptObject.cpp:1152 in 871a891. [](commit_id = 871a891, deletion_comment = False) |
|
@dotnet-bot test Windows x64_release |
Can we update the comment here? I'll try to enumerate combinations of return value I know. Please let me know if I missed something: Refers to: lib/Runtime/Types/JavascriptEnumerator.h:32 in 871a891. [](commit_id = 871a891, deletion_comment = False) |
|
|
||
| if (enumerator->GetCurrentPropertyId(&idMember)) | ||
| if (idMember != Js::Constants::NoProperty) | ||
| { | ||
| Js::Var newElement = Walk(Js::JavascriptString::FromVar(propertyNameVar), idMember, value); | ||
| if (Js::JavascriptOperators::IsUndefinedObject(newElement, undefined)) |
There was a problem hiding this comment.
should we reverse the check order (IsUndefined first before casting to JavascriptString? #Resolved
There was a problem hiding this comment.
huh? We are checking for undefined on newElement, and we are not casting that to string. #Resolved
I am planning to refactor is next and change the behavior. So I will document it then. In reply to: 238279890 [](ancestors = 238279890) Refers to: lib/Runtime/Types/JavascriptEnumerator.h:32 in 871a891. [](commit_id = 871a891, deletion_comment = False) |
|
|
Merge pull request #1400 from curtisman:enumclean The JavascriptEnumerator function GetCurrentAndMoveNext is misleading because. Because it move to the next one before return the next element that we moved to. Change to MoveAndGetNext. Also remove MoveNext/GetCurrentIndex/GetCurrentPropertyId, which is equivalent to GetCurrentAndMoveNext. Move all usage of those API to MoveAndGetNext.
The JavascriptEnumerator function GetCurrentAndMoveNext is misleading because. Because it move to the next one before return the next element that we moved to. Change to MoveAndGetNext.
Also remove MoveNext/GetCurrentIndex/GetCurrentPropertyId, which is equivalent to GetCurrentAndMoveNext. Move all usage of those API to MoveAndGetNext.