Skip to content

Commit 2d21577

Browse files
curtismancurtisman
committed
[MERGE #1400 @curtisman] Cleanup JavascriptEnumerator interface
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.
2 parents 797f40d + 871a891 commit 2d21577

37 files changed

+115
-560
lines changed

‎lib/Runtime/Base/CrossSiteEnumerator.h

Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -28,44 +28,25 @@ namespace Js
2828
DEFINE_VTABLE_CTOR(CrossSiteEnumerator<T>, T);
2929

3030
public:
31-
virtual Var GetCurrentIndex() override;
3231
virtual void Reset() override;
33-
virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override;
34-
virtual Var GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
32+
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
3533
virtual BOOL IsCrossSiteEnumerator() override
3634
{
3735
return true;
3836
}
3937

4038
};
4139

42-
template<typename T>
43-
Var CrossSiteEnumerator<T>::GetCurrentIndex()
44-
{
45-
Var result = __super::GetCurrentIndex();
46-
if (result)
47-
{
48-
result = CrossSite::MarshalVar(this->GetScriptContext(), result);
49-
}
50-
return result;
51-
}
52-
53-
template <typename T>
54-
BOOL CrossSiteEnumerator<T>::MoveNext(PropertyAttributes* attributes)
55-
{
56-
return __super::MoveNext(attributes);
57-
}
58-
5940
template <typename T>
6041
void CrossSiteEnumerator<T>::Reset()
6142
{
6243
__super::Reset();
6344
}
6445

6546
template <typename T>
66-
Var CrossSiteEnumerator<T>::GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes)
47+
Var CrossSiteEnumerator<T>::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
6748
{
68-
Var result = __super::GetCurrentAndMoveNext(propertyId, attributes);
49+
Var result = __super::MoveAndGetNext(propertyId, attributes);
6950
if (result)
7051
{
7152
result = CrossSite::MarshalVar(this->GetScriptContext(), result);

‎lib/Runtime/ByteCode/ByteCodeEmitter.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9116,7 +9116,7 @@ void EmitForIn(ParseNode *loopNode,
91169116
// The EndStatement will happen in the EmitForInOfLoopBody function
91179117
byteCodeGenerator->StartStatement(loopNode->sxForInOrForOf.pnodeLval);
91189118

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

91229122
EmitForInOfLoopBody(loopNode, loopEntrance, continuePastLoop, byteCodeGenerator, funcInfo, fReturnValue);

‎lib/Runtime/Debug/DiagObjectModel.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2063,7 +2063,8 @@ namespace Js
20632063
if (object->CanHaveInterceptors())
20642064
{
20652065
Js::ForInObjectEnumerator enumerator(object, object->GetScriptContext(), /* enumSymbols */ true);
2066-
if (enumerator.MoveNext())
2066+
Js::PropertyId propertyId;
2067+
if (enumerator.MoveAndGetNext(propertyId))
20672068
{
20682069
enumerator.Clear();
20692070
return TRUE;
@@ -2378,7 +2379,7 @@ namespace Js
23782379
Js::PropertyId propertyId;
23792380
Var obj;
23802381

2381-
while ((obj = enumerator->GetCurrentAndMoveNext(propertyId)) != nullptr)
2382+
while ((obj = enumerator->MoveAndGetNext(propertyId)) != nullptr)
23822383
{
23832384
if (!JavascriptString::Is(obj))
23842385
{
@@ -2402,7 +2403,7 @@ namespace Js
24022403
propertyId = propertyRecord->GetPropertyId();
24032404
}
24042405
}
2405-
// GetCurrentAndMoveNext shouldn't return an internal property id
2406+
// MoveAndGetNext shouldn't return an internal property id
24062407
Assert(!Js::IsInternalPropertyId(propertyId));
24072408

24082409
uint32 indexVal;

‎lib/Runtime/Language/JavascriptOperators.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5214,7 +5214,7 @@ namespace Js
52145214
Var JavascriptOperators::OP_BrOnEmpty(ForInObjectEnumerator * aEnumerator)
52155215
{
52165216
PropertyId id;
5217-
return aEnumerator->GetCurrentAndMoveNext(id);
5217+
return aEnumerator->MoveAndGetNext(id);
52185218
}
52195219

52205220
ForInObjectEnumerator * JavascriptOperators::OP_GetForInEnumerator(Var enumerable, ScriptContext* scriptContext)

‎lib/Runtime/Library/ArgumentsObjectEnumerator.cpp

Lines changed: 17 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -15,30 +15,22 @@ namespace Js
1515
Reset();
1616
}
1717

18-
Var ArgumentsObjectEnumerator::GetCurrentIndex()
19-
{
20-
if (!doneFormalArgs)
21-
{
22-
return argumentsObject->GetScriptContext()->GetIntegerString(formalArgIndex);
23-
}
24-
return objectEnumerator->GetCurrentIndex();
25-
}
26-
27-
BOOL ArgumentsObjectEnumerator::MoveNext(PropertyAttributes* attributes)
18+
Var ArgumentsObjectEnumerator::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
2819
{
2920
if (!doneFormalArgs)
3021
{
3122
formalArgIndex = argumentsObject->GetNextFormalArgIndex(formalArgIndex, this->enumNonEnumerable, attributes);
3223
if (formalArgIndex != JavascriptArray::InvalidIndex
3324
&& formalArgIndex < argumentsObject->GetNumberOfArguments())
3425
{
35-
return true;
26+
propertyId = Constants::NoProperty;
27+
return argumentsObject->GetScriptContext()->GetIntegerString(formalArgIndex);
3628
}
3729

3830
doneFormalArgs = true;
3931
}
40-
return objectEnumerator->MoveNext(attributes);
41-
}
32+
return objectEnumerator->MoveAndGetNext(propertyId, attributes);
33+
}
4234

4335
void ArgumentsObjectEnumerator::Reset()
4436
{
@@ -50,16 +42,6 @@ namespace Js
5042
objectEnumerator = (Js::JavascriptEnumerator*)enumerator;
5143
}
5244

53-
bool ArgumentsObjectEnumerator::GetCurrentPropertyId(PropertyId *pPropertyId)
54-
{
55-
if (!doneFormalArgs)
56-
{
57-
*pPropertyId = Constants::NoProperty;
58-
return false;
59-
}
60-
return objectEnumerator->GetCurrentPropertyId(pPropertyId);
61-
}
62-
6345
//---------------------- ES5ArgumentsObjectEnumerator -------------------------------
6446

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

72-
BOOL ES5ArgumentsObjectEnumerator::MoveNext(PropertyAttributes* attributes)
54+
Var ES5ArgumentsObjectEnumerator::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
7355
{
7456
// Formals:
7557
// - deleted => not in objectArray && not connected -- do not enum, do not advance
@@ -80,24 +62,24 @@ namespace Js
8062
{
8163
ES5HeapArgumentsObject* es5HAO = static_cast<ES5HeapArgumentsObject*>(argumentsObject);
8264
formalArgIndex = es5HAO->GetNextFormalArgIndexHelper(formalArgIndex, this->enumNonEnumerable, attributes);
83-
if (formalArgIndex != JavascriptArray::InvalidIndex)
84-
{
85-
if (formalArgIndex < argumentsObject->GetNumberOfArguments())
65+
if (formalArgIndex != JavascriptArray::InvalidIndex
66+
&& formalArgIndex < argumentsObject->GetNumberOfArguments())
67+
{
68+
if (argumentsObject->HasObjectArrayItem(formalArgIndex))
8669
{
87-
if (argumentsObject->HasObjectArrayItem(formalArgIndex))
88-
{
89-
BOOL tempResult = objectEnumerator->MoveNext(attributes);
90-
AssertMsg(tempResult, "We advanced objectEnumerator->MoveNext() too many times.");
91-
}
92-
93-
return TRUE;
70+
PropertyId tempPropertyId;
71+
Var tempIndex = objectEnumerator->MoveAndGetNext(tempPropertyId, attributes);
72+
AssertMsg(tempIndex, "We advanced objectEnumerator->MoveNext() too many times.");
9473
}
74+
75+
propertyId = Constants::NoProperty;
76+
return argumentsObject->GetScriptContext()->GetIntegerString(formalArgIndex);
9577
}
9678

9779
doneFormalArgs = true;
9880
}
9981

100-
return objectEnumerator->MoveNext(attributes);
82+
return objectEnumerator->MoveAndGetNext(propertyId, attributes);
10183
}
10284

10385
void ES5ArgumentsObjectEnumerator::Reset()

‎lib/Runtime/Library/ArgumentsObjectEnumerator.h

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,8 @@ namespace Js
2222

2323
public:
2424
ArgumentsObjectEnumerator(ArgumentsObject* argumentsObject, ScriptContext* requestcontext, BOOL enumNonEnumerable, bool enumSymbols = false);
25-
virtual Var GetCurrentIndex() override;
26-
virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override;
2725
virtual void Reset() override;
28-
virtual bool GetCurrentPropertyId(PropertyId *propertyId) override;
26+
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
2927
};
3028

3129
class ES5ArgumentsObjectEnumerator : public ArgumentsObjectEnumerator
@@ -36,10 +34,8 @@ namespace Js
3634

3735
public:
3836
ES5ArgumentsObjectEnumerator(ArgumentsObject* argumentsObject, ScriptContext* requestcontext, BOOL enumNonEnumerable, bool enumSymbols = false);
39-
40-
virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override;
4137
virtual void Reset() override;
42-
38+
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
4339
private:
4440
uint enumeratedFormalsInObjectArrayCount; // The number of enumerated formals for far.
4541
};

‎lib/Runtime/Library/ES5ArrayEnumerator.cpp

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,21 +17,7 @@ namespace Js
1717
Reset();
1818
}
1919

20-
Var ES5ArrayEnumerator::GetCurrentIndex()
21-
{
22-
if (!doneArray && index != JavascriptArray::InvalidIndex)
23-
{
24-
return arrayObject->GetScriptContext()->GetIntegerString(index);
25-
}
26-
else if (!doneObject)
27-
{
28-
return objectEnumerator->GetCurrentIndex();
29-
}
30-
31-
return GetLibrary()->GetUndefined();
32-
}
33-
34-
Var ES5ArrayEnumerator::GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes)
20+
Var ES5ArrayEnumerator::MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes)
3521
{
3622
propertyId = Constants::NoProperty;
3723

@@ -77,7 +63,7 @@ namespace Js
7763
}
7864
if (!doneObject)
7965
{
80-
Var currentIndex = objectEnumerator->GetCurrentAndMoveNext(propertyId, attributes);
66+
Var currentIndex = objectEnumerator->MoveAndGetNext(propertyId, attributes);
8167
if (currentIndex)
8268
{
8369
return currentIndex;

‎lib/Runtime/Library/ES5ArrayEnumerator.h

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,7 @@ namespace Js
2424

2525
public:
2626
ES5ArrayEnumerator(Var originalInstance, ES5Array* arrayObject, ScriptContext* scriptContext, BOOL enumNonEnumerable, bool enumSymbols = false);
27-
virtual Var GetCurrentIndex() override;
2827
virtual void Reset() override;
29-
virtual Var GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
28+
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr) override;
3029
};
3130
}

‎lib/Runtime/Library/ForInObjectEnumerator.cpp

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -136,12 +136,7 @@ namespace Js
136136

137137
Var ForInObjectEnumerator::GetCurrentIndex()
138138
{
139-
if (currentIndex)
140-
{
141-
return currentIndex;
142-
}
143-
Assert(currentEnumerator != nullptr);
144-
return currentEnumerator->GetCurrentIndex();
139+
return currentIndex;
145140
}
146141

147142
BOOL ForInObjectEnumerator::TestAndSetEnumerated(PropertyId propertyId)
@@ -155,11 +150,11 @@ namespace Js
155150
BOOL ForInObjectEnumerator::MoveNext()
156151
{
157152
PropertyId propertyId;
158-
currentIndex = GetCurrentAndMoveNext(propertyId);
153+
currentIndex = MoveAndGetNext(propertyId);
159154
return currentIndex != NULL;
160155
}
161156

162-
Var ForInObjectEnumerator::GetCurrentAndMoveNext(PropertyId& propertyId)
157+
Var ForInObjectEnumerator::MoveAndGetNext(PropertyId& propertyId)
163158
{
164159
JavascriptEnumerator *pEnumerator = currentEnumerator;
165160
PropertyRecord const * propRecord;
@@ -168,7 +163,7 @@ namespace Js
168163
while (true)
169164
{
170165
propertyId = Constants::NoProperty;
171-
currentIndex = pEnumerator->GetCurrentAndMoveNext(propertyId, &attributes);
166+
currentIndex = pEnumerator->MoveAndGetNext(propertyId, &attributes);
172167
#if ENABLE_COPYONACCESS_ARRAY
173168
JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(currentIndex);
174169
#endif
@@ -177,7 +172,7 @@ namespace Js
177172
if (firstPrototype == nullptr)
178173
{
179174
// We are calculating correct shadowing for non-enumerable properties of the child object, we will receive
180-
// both enumerable and non-enumerable properties from GetCurrentAndMoveNext so we need to check before we simply
175+
// both enumerable and non-enumerable properties from MoveAndGetNext so we need to check before we simply
181176
// return here. If this property is non-enumerable we're going to skip it.
182177
if (!(attributes & PropertyEnumerable))
183178
{

‎lib/Runtime/Library/ForInObjectEnumerator.h

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ namespace Js
3939
Var GetCurrentIndex();
4040
BOOL MoveNext();
4141
void Reset();
42-
Var GetCurrentAndMoveNext(PropertyId& propertyId);
42+
Var MoveAndGetNext(PropertyId& propertyId);
4343

4444
static uint32 GetOffsetOfCurrentEnumerator() { return offsetof(ForInObjectEnumerator, currentEnumerator); }
4545
static uint32 GetOffsetOfFirstPrototype() { return offsetof(ForInObjectEnumerator, firstPrototype); }
@@ -56,12 +56,10 @@ namespace Js
5656
{
5757
}
5858

59-
virtual Var GetCurrentIndex() override { return forInObjectEnumerator.GetCurrentIndex(); }
60-
virtual BOOL MoveNext(PropertyAttributes* attributes = nullptr) override { return forInObjectEnumerator.MoveNext(); }
6159
virtual void Reset() override { forInObjectEnumerator.Reset(); }
62-
virtual Var GetCurrentAndMoveNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr)
60+
virtual Var MoveAndGetNext(PropertyId& propertyId, PropertyAttributes* attributes = nullptr)
6361
{
64-
return forInObjectEnumerator.GetCurrentAndMoveNext(propertyId);
62+
return forInObjectEnumerator.MoveAndGetNext(propertyId);
6563
}
6664
protected:
6765
DEFINE_VTABLE_CTOR(ForInObjectEnumeratorWrapper, JavascriptEnumerator);

0 commit comments

Comments
 (0)