[PATCH] D23649: [ADT] Change the range-based for loop in DFS to normal loop. NFC.

[Tim Shen via llvm-commits]

On Thu, Aug 18, 2016 at 2:26 PM David Blaikie <dblaikie at gmail.com> wrote:

> On Thu, Aug 18, 2016 at 1:35 AM Tim Shen <timshen at google.com> wrote:
>
>> timshen created this revision.
>> timshen added a reviewer: dblaikie.
>> timshen added a subscriber: llvm-commits.
>>
>> Using range-based for loop copies the iterator, so it ends up not
>> updating *Opt.
>>
>> Before the change, the DFS result is correct, because Visited keeps track
>> of visited nodes. It's just DFS is slower, because for every time
>> toNext() is called, the for loop starts over at child_begin().
>>
>
> This description doesn't quite make sense to me - the loop doesn't appear
> to start over at the beginning. The change causes the loop to go to the
> initial end, rather than the new end (by caching the original end, rather
> than testing against the current/new end on each iteration)
>
> Why are new nodes appearing in the graph through the iteration?
>
> (or I'm misunderstanding this in some way(s))
>

There are several things mixed together:
1) DepthFirstIteartor does explicitly support graph mutation during the
iteration. See r76869.
2) I didn't notice that with the range-based for loop, end is cached. But
not caching the end seems to be the right thing to do.
3) More importantly, the purpose of this patch is to use "auto &It = *Opt",
not "auto It = Opt", as range-based for loop will do. So that
Visited.back().second is actually updated before toNext() exits.


>
>
>>
>> https://reviews.llvm.org/D23649
>>
>> Files:
>>   include/llvm/ADT/DepthFirstIterator.h
>>
>> Index: include/llvm/ADT/DepthFirstIterator.h
>> ===================================================================
>> --- include/llvm/ADT/DepthFirstIterator.h
>> +++ include/llvm/ADT/DepthFirstIterator.h
>> @@ -107,7 +107,8 @@
>>        if (!Opt)
>>          Opt.emplace(GT::child_begin(Node));
>>
>> -      for (NodeRef Next : make_range(*Opt, GT::child_end(Node))) {
>> +      for (auto &It = *Opt; It != GT::child_end(Node); ++It) {
>> +        NodeRef Next = *It;
>>          // Has our next sibling been visited?
>>          if (this->Visited.insert(Next).second) {
>>            // No, do it now.
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160819/37bcec65/attachment.html>