[PATCH] D23649: [ADT] Actually mutate the iterator VisitStack.back().second, not its copy.

[David Blaikie via llvm-commits]

On Mon, Aug 22, 2016 at 2:42 PM Tim Shen <timshen at google.com> wrote:

> timshen added inline comments.
>
> ================
> Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:22
> @@ +21,3 @@
> +public:
> +  using iterator = typename SmallPtrSet<T, 4>::iterator;
> +
> ----------------
> dblaikie wrote:
> > Typedef?
> When do we use using and when typedef? If I'm creating a new file I'd go
> for using.
>

Pretty sure we use typedef wherever we can - if the style guide says
differently, happy to be corrected. (or happy to have a style guide
discussion about changing the direction here)


>
> ================
> Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:51-53
> @@ +50,5 @@
> +  StorageT S;
> +  for (auto N : make_range(DFIter::begin(G, S), DFIter::end(G, S))) {
> +    (void)N;
> +  }
> +  EXPECT_EQ(3, S.InsertVisited);
> ----------------
> dblaikie wrote:
> > Probably drop braces on a single line loop.
> >
> > Though maybe this is better expressed with a different construct?
> >
> >   (void)std::distance(begin, end), perhaps?
> >
> > (saves the make_range, extra variable copies, etc?) Probably with a
> comment saying something like "walk the DF ordering & ensure each element
> was only visited once"
> std::distance doesn't do what we want on randomaccess iterators,


Fair point


> so using std::distance makes it depends more on the context. I'd rather
> make it more explicit using range-based for loop.
>

Still a lot under the covers with a range-for. But whatever works for you.

Commit when ready,

- Dave


>
>
> https://reviews.llvm.org/D23649
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160822/cf78c9bc/attachment.html>