Bug 166060 – Modernize for loops in JSC

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Konstantin Tokarev]

Modernize for loops in JSC

[Konstantin Tokarev]

Created attachment 297519 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 297519 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297519&action=review

r=me with comments.

> Source/JavaScriptCore/API/JSCallbackObject.h:111
> +            for (auto& ptr : m_propertyMap) {
> +                if (ptr.value)
> +                    visitor.append(ptr.value);

Use the name like `pair` since it is no longer pointer.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:4068
> +        for (auto& exit : jitCode->osrExit) {
>              exit.considerAddingAsFrequentExitSite(profiledBlock);
>          }

Drop this brace.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:205
> +    for (FunctionMetadataNode* function : functionStack) {
>          m_functionsToInitialize.append(std::make_pair(function, GlobalFunctionVariable));
>      }

Drop this brace. And use `auto*` instead.

> Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp:294
> +        for (Node* node : *m_block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1422
> +        for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGIntegerCheckCombiningPhase.cpp:195
> +        for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGIntegerRangeOptimizationPhase.cpp:1112
> +                for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGLICMPhase.cpp:105
> +            for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGMaximalFlushInsertionPhase.cpp:111
> +        for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp:563
> +            for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1772
> +        for (auto& info : m_generationInfo) {
>              RELEASE_ASSERT(!info.alive());
>          }

Drop this brace.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8896
> +    for (BranchRecord& branch : m_branches) {
>          branch.jump.linkTo(m_jit.blockHeads()[branch.destination->index], &m_jit);
>      }

Drop this brace. And use `auto&`.

> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:74
> +            for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp:275
> +                    for (Node* subNode : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp:307
> +            for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGTypeCheckHoistingPhase.cpp:365
> +                    for (auto subNode : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGValidate.cpp:641
> +            for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/dfg/DFGVirtualRegisterAllocationPhase.cpp:62
> +            for (Node* node : *block) {

Let's use `auto*`.

> Source/JavaScriptCore/heap/HeapVerifier.cpp:149
> +    for (auto& objData : after.liveObjects) {
>          knownLiveSet.add(objData.obj);
>      }

Drop this brace.

> Source/JavaScriptCore/heap/MarkedAllocator.cpp:54
> +    for (MarkedBlock::Handle* block : m_blocks) {

Let's use `auto*`.

> Source/JavaScriptCore/jit/JIT.cpp:696
> +    for (SwitchRecord record : m_switches) {

Let's use `auto&`

> Source/JavaScriptCore/jit/JIT.cpp:698
>  

In the following code, we have the similar for statements. One for branchOffsets and one for offsetTable.
I think we can change them too.

> Source/JavaScriptCore/runtime/FunctionHasExecutedCache.cpp:41
> +    for (auto& iter : map) {

Instead of using `iter`, let's use `pair` since it is no longer iterator.

> Source/JavaScriptCore/runtime/FunctionHasExecutedCache.cpp:91
> +    for (auto& iter : map) {

Instead of using `iter`, let's use `pair` since it is no longer iterator.

> Source/JavaScriptCore/runtime/TypeProfiler.cpp:125
> +    for (TypeLocation* location : bucket) {

Use `auto*`.

> Source/JavaScriptCore/runtime/TypeSet.cpp:61
> +            for (RefPtr<StructureShape>& seenShape : m_structureHistory) {

Use `auto&`.

> Source/JavaScriptCore/runtime/TypeSet.cpp:116
> +    for (const RefPtr<StructureShape>& shape : m_structureHistory) {

Use `const auto&` here.

> Source/JavaScriptCore/runtime/WeakMapData.cpp:115
> +    for (auto& it : m_target->m_map) {
> +        if (!Heap::isMarked(it.key))
>              continue;
>          m_liveKeyCount++;
> -        visitor.append(it->value);
> +        visitor.append(it.value);

You can rename `it` to something like `pair` since it is no longer iterator.

> Source/JavaScriptCore/runtime/WeakMapData.cpp:133
> +        for (auto& it : m_target->m_map) {
> +            if (Heap::isMarked(it.key))
>                  continue;
> -            deadEntries.uncheckedAppend(it->key);
> +            deadEntries.uncheckedAppend(it.key);
>          }

You can rename `it` to something like `pair` since it is no longer iterator.

> Source/JavaScriptCore/runtime/WeakMapData.cpp:134
> +        for (auto& deadEntrie : deadEntries)

/deadEntrie/deadEntry/

> Source/JavaScriptCore/runtime/WeakMapData.cpp:141
> +        for (auto& it : m_target->m_map) {
> +            if (!Heap::isMarked(it.key))
>                  continue;
> -            newMap.add(it->key, it->value);
> +            newMap.add(it.key, it.value);

You can rename `it` to something like `pair` since it is no longer iterator.

> Source/JavaScriptCore/tools/ProfileTreeNode.h:91
> +        for (MapEntry* entry : entries) {

Use `auto*`.

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:1946
> +            for (PatternTerm& term : alternative->m_terms) {

Use `auto&`.

[Konstantin Tokarev]

Comment on attachment 297519 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297519&action=review

>> Source/JavaScriptCore/API/JSCallbackObject.h:111
>> +                    visitor.append(ptr.value);
> 
> Use the name like `pair` since it is no longer pointer.

Done

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:4068
>>          }
> 
> Drop this brace.

Done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:205
>>      }
> 
> Drop this brace. And use `auto*` instead.

Done

>> Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp:294
>> +        for (Node* node : *m_block) {
> 
> Let's use `auto*`.

I thought that because both "auto" and "Node" are both 4-character identifiers, there is little incentive to prefer "auto" in places like this one

>> Source/JavaScriptCore/heap/HeapVerifier.cpp:149
>>      }
> 
> Drop this brace.

Done

>> Source/JavaScriptCore/heap/MarkedAllocator.cpp:54
>> +    for (MarkedBlock::Handle* block : m_blocks) {
> 
> Let's use `auto*`.

Done

>> Source/JavaScriptCore/jit/JIT.cpp:698
>>  
> 
> In the following code, we have the similar for statements. One for branchOffsets and one for offsetTable.
> I think we can change them too.

Loop at 705 cannot be converted straightforwardly because it operates with 2 parallel Vectors

Done for loops at 714 and 726

>> Source/JavaScriptCore/runtime/FunctionHasExecutedCache.cpp:41
>> +    for (auto& iter : map) {
> 
> Instead of using `iter`, let's use `pair` since it is no longer iterator.

Done

>> Source/JavaScriptCore/runtime/FunctionHasExecutedCache.cpp:91
>> +    for (auto& iter : map) {
> 
> Instead of using `iter`, let's use `pair` since it is no longer iterator.

Done

>> Source/JavaScriptCore/runtime/TypeProfiler.cpp:125
>> +    for (TypeLocation* location : bucket) {
> 
> Use `auto*`.

Done

>> Source/JavaScriptCore/runtime/TypeSet.cpp:61
>> +            for (RefPtr<StructureShape>& seenShape : m_structureHistory) {
> 
> Use `auto&`.

Done

>> Source/JavaScriptCore/runtime/TypeSet.cpp:116
>> +    for (const RefPtr<StructureShape>& shape : m_structureHistory) {
> 
> Use `const auto&` here.

Done

>> Source/JavaScriptCore/runtime/WeakMapData.cpp:115
>> +        visitor.append(it.value);
> 
> You can rename `it` to something like `pair` since it is no longer iterator.

Done

>> Source/JavaScriptCore/runtime/WeakMapData.cpp:133
>>          }
> 
> You can rename `it` to something like `pair` since it is no longer iterator.

Done

>> Source/JavaScriptCore/runtime/WeakMapData.cpp:134
>> +        for (auto& deadEntrie : deadEntries)
> 
> /deadEntrie/deadEntry/

Done

>> Source/JavaScriptCore/runtime/WeakMapData.cpp:141
>> +            newMap.add(it.key, it.value);
> 
> You can rename `it` to something like `pair` since it is no longer iterator.

Done

>> Source/JavaScriptCore/tools/ProfileTreeNode.h:91
>> +        for (MapEntry* entry : entries) {
> 
> Use `auto*`.

Done

>> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:1946
>> +            for (PatternTerm& term : alternative->m_terms) {
> 
> Use `auto&`.

Done

[Konstantin Tokarev]

Created attachment 297525 [details]
Patch

[Konstantin Tokarev]

Changed "Node* node" to use auto* as well

[WebKit Commit Bot]

Comment on attachment 297525 [details]
Patch

Clearing flags on attachment: 297525

Committed r210023: <http://trac.webkit.org/changeset/210023>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.