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

[David Blaikie via llvm-commits]

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))


>
> 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/20160818/f9516732/attachment.html>