[PATCH] Minor improvements to scc_iterator

[Duncan P. N. Exon Smith]

On 2014 Feb 1, at 19:35, Mehdi Amini <mehdi.amini at silkan.com> wrote:

>  Update, thanks to review from Duncan P. N. Exon Smith:
> 
>  - naming convention

Sorry Mehdi, I was unclear and you got the wrong idea.

- It’s best *not* to update old names in a file in the same patch/commit as a
  real change to the code.   Doing so makes it difficult to differentiate
  between stylistic vs. semantic changes.
- If you *really* want to update the style file-wide, that should be a separate
  patch, although the value of that is debatable.  It adds a hurdle for later
  developers doing a blame.

Nevertheless, your *new* variables should follow the standard style.

Please resubmit without changing the naming of variables already in the file.

>  - replaced std::tuple with a struct.

This looks much better.  Awesome!  However:

> Index: include/llvm/ADT/SCCIterator.h
> ===================================================================
> --- include/llvm/ADT/SCCIterator.h
> +++ include/llvm/ADT/SCCIterator.h
> @@ -25,6 +25,7 @@
>  
>  #include "llvm/ADT/DenseMap.h"
>  #include "llvm/ADT/GraphTraits.h"
> +#include <tuple>

You don’t need #include <tuple> anymore.

>  I noticed a minor semantical change with respect to the original code: iterator equality is implemented the following way:
> 
>    inline bool operator==(const scc_iterator &x) const {
>      return VisitStack == x.VisitStack && CurrentSCC == x.CurrentSCC;
>    }
> 
>  Previously the MinVisited value was stored on a separated stack and not checked for equality, now it is.

I had a look, and this semantic change seems fine.  Consumers *should* be using
scc_iterator::isAtEnd() rather than comparing to ::end() anyway.