[PATCH] D32682: Refactoring with range-based for, NFC

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D32682#743280, @chenwj wrote:

> In https://reviews.llvm.org/D32682#742714, @spatel wrote:
>
> > IMO, this is going beyond LLVM's normal 'auto' usage prefs:
> >  http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> >
> > If you can convert it to a range-loop, then you should specify the range element type including 'const' where appropriate. So something like this:
> >
> >   for (const SDValue &Op : NI->op_values())
> >   
> >
> > Some of the diffs use regular loops and only replace the iterator type with an 'auto'. That should be a separate NFC patch. But it's not clear to me if that also goes too far into 'auto'.
>
>
> By beyond, you mean those originally were `const_iterator` or all of them? FWIK, to make `auto` as `const_iterator`, we need the container provides `cbegin()/cend()`. Or you know other way to make it happen?
>
> Thanks.


By "beyond LLVM's normal 'auto' usage prefs", I mean that we would generally prefer this:

  for (SDep &Pred : SU->Preds) {

instead of this:

  for (auto &Pred : SU->Preds) {

because it makes the code clearer to state the element type. That's the first diff in this patch.

The const-ness is a separate issue. If you can use this:

  for (const SDep &Pred : SU->Preds) {

Then that is even better because a reader of the code can immediately know that Pred is not being written in that loop.

FWIW, I don't think I've ever made changes to these files, so if there's some reason to differ from the normal LLVM usage here, let me know.


https://reviews.llvm.org/D32682