Skip to content

Cleanup JavascriptEnumerator interface #1400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 8, 2016
Merged

Cleanup JavascriptEnumerator interface #1400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 3 additions & 22 deletions lib/Runtime/Base/CrossSiteEnumerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,44 +28,25 @@ namespace Js
DEFINE_VTABLE_CTOR(CrossSiteEnumerator<T>, T);

public:
virtual Var GetCurrentIndex() override;
virtual void Reset() override;
virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override;
virtual Var GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
virtual BOOL IsCrossSiteEnumerator() override
{
return true;
}

};

template<typename T>
Var CrossSiteEnumerator<T>::GetCurrentIndex()
{
Var result = __super::GetCurrentIndex();
if (result)
{
result = CrossSite::MarshalVar(this->GetScriptContext(), result);
}
return result;
}

template <typename T>
BOOL CrossSiteEnumerator<T>::MoveNext(PropertyAttributes* attributes)
{
return __super::MoveNext(attributes);
}

template <typename T>
void CrossSiteEnumerator<T>::Reset()
{
__super::Reset();
}

template <typename T>
Var CrossSiteEnumerator<T>::GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes)
Var CrossSiteEnumerator<T>::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
{
Var result = __super::GetCurrentAndMoveNext(propertyId, attributes);
Var result = __super::MoveAndGetNext(propertyId, attributes);
if (result)
{
result = CrossSite::MarshalVar(this->GetScriptContext(), result);
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/ByteCode/ByteCodeEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9116,7 +9116,7 @@ void EmitForIn(ParseNode *loopNode,
// The EndStatement will happen in the EmitForInOfLoopBody function
byteCodeGenerator->StartStatement(loopNode->sxForInOrForOf.pnodeLval);

// branch past loop when GetCurrentAndMoveNext returns nullptr
// branch past loop when MoveAndGetNext returns nullptr
byteCodeGenerator->Writer()->BrReg2(Js::OpCode::BrOnEmpty, continuePastLoop, loopNode->sxForInOrForOf.itemLocation, loopNode->location);

EmitForInOfLoopBody(loopNode, loopEntrance, continuePastLoop, byteCodeGenerator, funcInfo, fReturnValue);
Expand Down
7 changes: 4 additions & 3 deletions lib/Runtime/Debug/DiagObjectModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2063,7 +2063,8 @@ namespace Js
if (object->CanHaveInterceptors())
{
Js::ForInObjectEnumerator enumerator(object, object->GetScriptContext(), /* enumSymbols */ true);
if (enumerator.MoveNext())
Js::PropertyId propertyId;
if (enumerator.MoveAndGetNext(propertyId))
{
enumerator.Clear();
return TRUE;
Expand Down Expand Up @@ -2378,7 +2379,7 @@ namespace Js
Js::PropertyId propertyId;
Var obj;

while ((obj = enumerator->GetCurrentAndMoveNext(propertyId)) != nullptr)
while ((obj = enumerator->MoveAndGetNext(propertyId)) != nullptr)
{
if (!JavascriptString::Is(obj))
{
Expand All @@ -2402,7 +2403,7 @@ namespace Js
propertyId = propertyRecord->GetPropertyId();
}
}
// GetCurrentAndMoveNext shouldn't return an internal property id
// MoveAndGetNext shouldn't return an internal property id
Assert(!Js::IsInternalPropertyId(propertyId));

uint32 indexVal;
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Language/JavascriptOperators.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5214,7 +5214,7 @@ namespace Js
Var JavascriptOperators::OP_BrOnEmpty(ForInObjectEnumerator * aEnumerator)
{
PropertyId id;
return aEnumerator->GetCurrentAndMoveNext(id);
return aEnumerator->MoveAndGetNext(id);
}

ForInObjectEnumerator * JavascriptOperators::OP_GetForInEnumerator(Var enumerable, ScriptContext* scriptContext)
Expand Down
52 changes: 17 additions & 35 deletions lib/Runtime/Library/ArgumentsObjectEnumerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,30 +15,22 @@ namespace Js
Reset();
}

Var ArgumentsObjectEnumerator::GetCurrentIndex()
{
if (!doneFormalArgs)
{
return argumentsObject->GetScriptContext()->GetIntegerString(formalArgIndex);
}
return objectEnumerator->GetCurrentIndex();
}

BOOL ArgumentsObjectEnumerator::MoveNext(PropertyAttributes* attributes)
Var ArgumentsObjectEnumerator::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
{
if (!doneFormalArgs)
{
formalArgIndex = argumentsObject->GetNextFormalArgIndex(formalArgIndex, this->enumNonEnumerable, attributes);
if (formalArgIndex != JavascriptArray::InvalidIndex
&& formalArgIndex < argumentsObject->GetNumberOfArguments())
{
return true;
propertyId = Constants::NoProperty;
return argumentsObject->GetScriptContext()->GetIntegerString(formalArgIndex);
}

doneFormalArgs = true;
}
return objectEnumerator->MoveNext(attributes);
}
return objectEnumerator->MoveAndGetNext(propertyId, attributes);
}

void ArgumentsObjectEnumerator::Reset()
{
Expand All @@ -50,16 +42,6 @@ namespace Js
objectEnumerator = (Js::JavascriptEnumerator*)enumerator;
}

bool ArgumentsObjectEnumerator::GetCurrentPropertyId(PropertyId *pPropertyId)
{
if (!doneFormalArgs)
{
*pPropertyId = Constants::NoProperty;
return false;
}
return objectEnumerator->GetCurrentPropertyId(pPropertyId);
}

//---------------------- ES5ArgumentsObjectEnumerator -------------------------------

ES5ArgumentsObjectEnumerator::ES5ArgumentsObjectEnumerator(ArgumentsObject* argumentsObject, ScriptContext* requestcontext, BOOL enumNonEnumerable, bool enumSymbols)
Expand All @@ -69,7 +51,7 @@ namespace Js
this->Reset();
}

BOOL ES5ArgumentsObjectEnumerator::MoveNext(PropertyAttributes* attributes)
Var ES5ArgumentsObjectEnumerator::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
{
// Formals:
// - deleted => not in objectArray && not connected -- do not enum, do not advance
Expand All @@ -80,24 +62,24 @@ namespace Js
{
ES5HeapArgumentsObject* es5HAO = static_cast<ES5HeapArgumentsObject*>(argumentsObject);
formalArgIndex = es5HAO->GetNextFormalArgIndexHelper(formalArgIndex, this->enumNonEnumerable, attributes);
if (formalArgIndex != JavascriptArray::InvalidIndex)
{
if (formalArgIndex < argumentsObject->GetNumberOfArguments())
if (formalArgIndex != JavascriptArray::InvalidIndex
&& formalArgIndex < argumentsObject->GetNumberOfArguments())
{
if (argumentsObject->HasObjectArrayItem(formalArgIndex))
{
if (argumentsObject->HasObjectArrayItem(formalArgIndex))
{
BOOL tempResult = objectEnumerator->MoveNext(attributes);
AssertMsg(tempResult, "We advanced objectEnumerator->MoveNext() too many times.");
}

return TRUE;
PropertyId tempPropertyId;
Var tempIndex = objectEnumerator->MoveAndGetNext(tempPropertyId, attributes);
AssertMsg(tempIndex, "We advanced objectEnumerator->MoveNext() too many times.");
}

propertyId = Constants::NoProperty;
return argumentsObject->GetScriptContext()->GetIntegerString(formalArgIndex);
}

doneFormalArgs = true;
}

return objectEnumerator->MoveNext(attributes);
return objectEnumerator->MoveAndGetNext(propertyId, attributes);
}

void ES5ArgumentsObjectEnumerator::Reset()
Expand Down
8 changes: 2 additions & 6 deletions lib/Runtime/Library/ArgumentsObjectEnumerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,8 @@ namespace Js

public:
ArgumentsObjectEnumerator(ArgumentsObject* argumentsObject, ScriptContext* requestcontext, BOOL enumNonEnumerable, bool enumSymbols = false);
virtual Var GetCurrentIndex() override;
virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override;
virtual void Reset() override;
virtual bool GetCurrentPropertyId(PropertyId *propertyId) override;
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
};

class ES5ArgumentsObjectEnumerator : public ArgumentsObjectEnumerator
Expand All @@ -36,10 +34,8 @@ namespace Js

public:
ES5ArgumentsObjectEnumerator(ArgumentsObject* argumentsObject, ScriptContext* requestcontext, BOOL enumNonEnumerable, bool enumSymbols = false);

virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override;
virtual void Reset() override;

virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
private:
uint enumeratedFormalsInObjectArrayCount; // The number of enumerated formals for far.
};
Expand Down
18 changes: 2 additions & 16 deletions lib/Runtime/Library/ES5ArrayEnumerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,7 @@ namespace Js
Reset();
}

Var ES5ArrayEnumerator::GetCurrentIndex()
{
if (!doneArray && index != JavascriptArray::InvalidIndex)
{
return arrayObject->GetScriptContext()->GetIntegerString(index);
}
else if (!doneObject)
{
return objectEnumerator->GetCurrentIndex();
}

return GetLibrary()->GetUndefined();
}

Var ES5ArrayEnumerator::GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes)
Var ES5ArrayEnumerator::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
{
propertyId = Constants::NoProperty;

Expand Down Expand Up @@ -77,7 +63,7 @@ namespace Js
}
if (!doneObject)
{
Var currentIndex = objectEnumerator->GetCurrentAndMoveNext(propertyId, attributes);
Var currentIndex = objectEnumerator->MoveAndGetNext(propertyId, attributes);
if (currentIndex)
{
return currentIndex;
Expand Down
3 changes: 1 addition & 2 deletions lib/Runtime/Library/ES5ArrayEnumerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@ namespace Js

public:
ES5ArrayEnumerator(Var originalInstance, ES5Array* arrayObject, ScriptContext* scriptContext, BOOL enumNonEnumerable, bool enumSymbols = false);
virtual Var GetCurrentIndex() override;
virtual void Reset() override;
virtual Var GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
};
}
15 changes: 5 additions & 10 deletions lib/Runtime/Library/ForInObjectEnumerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,7 @@ namespace Js

Var ForInObjectEnumerator::GetCurrentIndex()
{
if (currentIndex)
{
return currentIndex;
}
Assert(currentEnumerator != nullptr);
return currentEnumerator->GetCurrentIndex();
return currentIndex;
}

BOOL ForInObjectEnumerator::TestAndSetEnumerated(PropertyId propertyId)
Expand All @@ -155,11 +150,11 @@ namespace Js
BOOL ForInObjectEnumerator::MoveNext()
Copy link
Contributor

@Yongqu Yongqu Aug 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we going to change this sometime as well? #Resolved

Copy link
Contributor Author

@curtisman curtisman Aug 8, 2016 •
edited by Yongqu
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still used in Chakra.dll #Resolved

{
PropertyId propertyId;
currentIndex = GetCurrentAndMoveNext(propertyId);
currentIndex = MoveAndGetNext(propertyId);
return currentIndex != NULL;
}

Var ForInObjectEnumerator::GetCurrentAndMoveNext(PropertyId& propertyId)
Var ForInObjectEnumerator::MoveAndGetNext(PropertyId& propertyId)
{
JavascriptEnumerator *pEnumerator = currentEnumerator;
PropertyRecord const * propRecord;
Expand All @@ -168,7 +163,7 @@ namespace Js
while (true)
{
propertyId = Constants::NoProperty;
currentIndex = pEnumerator->GetCurrentAndMoveNext(propertyId, &attributes);
currentIndex = pEnumerator->MoveAndGetNext(propertyId, &attributes);
#if ENABLE_COPYONACCESS_ARRAY
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(currentIndex);
#endif
Expand All @@ -177,7 +172,7 @@ namespace Js
if (firstPrototype == nullptr)
{
// We are calculating correct shadowing for non-enumerable properties of the child object, we will receive
// both enumerable and non-enumerable properties from GetCurrentAndMoveNext so we need to check before we simply
// both enumerable and non-enumerable properties from MoveAndGetNext so we need to check before we simply
// return here. If this property is non-enumerable we're going to skip it.
if (!(attributes & PropertyEnumerable))
{
Expand Down
8 changes: 3 additions & 5 deletions lib/Runtime/Library/ForInObjectEnumerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ namespace Js
Var GetCurrentIndex();
BOOL MoveNext();
void Reset();
Var GetCurrentAndMoveNext(PropertyId& propertyId);
Var MoveAndGetNext(PropertyId& propertyId);

static uint32 GetOffsetOfCurrentEnumerator() { return offsetof(ForInObjectEnumerator, currentEnumerator); }
static uint32 GetOffsetOfFirstPrototype() { return offsetof(ForInObjectEnumerator, firstPrototype); }
Expand All @@ -56,12 +56,10 @@ namespace Js
{
}

virtual Var GetCurrentIndex() override { return forInObjectEnumerator.GetCurrentIndex(); }
virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override { return forInObjectEnumerator.MoveNext(); }
virtual void Reset() override { forInObjectEnumerator.Reset(); }
virtual Var GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr)
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr)
{
return forInObjectEnumerator.GetCurrentAndMoveNext(propertyId);
return forInObjectEnumerator.MoveAndGetNext(propertyId);
}
protected:
DEFINE_VTABLE_CTOR(ForInObjectEnumeratorWrapper, JavascriptEnumerator);
Expand Down
40 changes: 7 additions & 33 deletions lib/Runtime/Library/IteratorObjectEnumerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,43 +20,17 @@ namespace Js
iteratorObject = RecyclableObject::FromVar(iterator);
}

void IteratorObjectEnumerator::EnsureIterator()
{
if (value == nullptr)
{
MoveNext();
}
}

Var IteratorObjectEnumerator::GetCurrentIndex()
{
EnsureIterator();
if (done)
{
return GetScriptContext()->GetLibrary()->GetUndefined();
}
return value;
}

BOOL IteratorObjectEnumerator::MoveNext(PropertyAttributes* attributes)
Var IteratorObjectEnumerator::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
{
ScriptContext* scriptContext = GetScriptContext();
done = !JavascriptOperators::IteratorStepAndValue(iteratorObject, scriptContext, &value);

if (attributes != nullptr)
if (JavascriptOperators::IteratorStepAndValue(iteratorObject, scriptContext, &value))
{
*attributes = PropertyEnumerable;
}

return !done;
}
if (attributes != nullptr)
{
*attributes = PropertyEnumerable;
}

Var IteratorObjectEnumerator::GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes)
{
if (MoveNext(attributes))
{
Var currentIndex = GetCurrentIndex();
ScriptContext* scriptContext = GetScriptContext();
Var currentIndex = value;
const PropertyRecord* propertyRecord = nullptr;
if (!TaggedInt::Is(currentIndex) && JavascriptString::Is(currentIndex) &&
VirtualTableInfo<Js::PropertyString>::HasVirtualTable(JavascriptString::FromVar(currentIndex)))
Expand Down
4 changes: 1 addition & 3 deletions lib/Runtime/Library/IteratorObjectEnumerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ namespace Js
{
public:
static Var Create(ScriptContext* scriptContext, Var iteratorObject);
virtual Var GetCurrentIndex() override;
virtual Var GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr);
virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override;
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr);
virtual void Reset() override;
protected:
IteratorObjectEnumerator(ScriptContext* scriptContext, Var iteratorObject);
Expand Down
Loading