[Docs] Update CodingStandards to recommend range-based for loops

[asb]

Summary

The CodingStandards section on avoiding the re-evaluation of end() hasn't been updated since range-based for loops have been adopted in the LLVM codebase. This patch adds a very brief section that documents how range-based for loops should be used wherever possible. It also moves example code in CodingStandards to use range-based for loops and auto when appropriate.

[majnemer]

docs/CodingStandards.rst

944

I'd replace the auto with Instruction &.

964

Ditto.

1338

You removed text which mentioned the most common case was mutation while iteration, I think we should keep at least part of that exposition somewhere.

[aaron.ballman]

docs/CodingStandards.rst

944

We should clarify this in the coding standard, because I've always understood we generally *want* to use auto in range-based for loops, for brevity (and because the type is generally immediately apparent from the container).

[javed.absar]

docs/CodingStandards.rst

944

In some complicated cases not using 'auto' improves code understanding. I have had cases where well established reviewers of llvm have recommended me not to use 'auto' in their review comment.

[majnemer]

docs/CodingStandards.rst

944

The most consistent rule I have seen is that we want to avoid repeating ourselves. auto makes a lot of sense if you just made a std::vector<int> a few lines up and are now iterating. If you have some FancyContainerTy, it is less awesome to use auto as it is not clear what it is you get from iterating it.

[asb]

docs/CodingStandards.rst

944

For FancyContainerTy sure, but my impression is it's quite common to use auto for simple container types where the types are well known to the average LLVM developer. e.g. Instructions in a BasicBlock, BasicBlocks in a Function.

[aaron.ballman]

docs/CodingStandards.rst

944

Agreed that we use auto primarily when the type is explicitly spelled out in the initialization. I was mostly interested in clarification around range-based for loops. The above is an example where I usually recommend auto because I thought that was the direction we had picked back when the C++11 discussion happened. If that's an incorrect understanding, I think we really should clarify the coding standard so I have something more concrete to point to.

[aaron.ballman]

docs/CodingStandards.rst

944

For FancyContainerTy sure, but my impression is it's quite common to use auto for simple container types where the types are well known to the average LLVM developer. e.g. Instructions in a BasicBlock, BasicBlocks in a Function.

That's been my recommendation during coding reviews, which is why I was surprised to see the opposite suggested here.

[asb]

docs/CodingStandards.rst

944

I was trying to comment specifically on range-based for loops. My comment should have said "it's quite common to use auto for iterating over simple container types...". It could obviously be argued that instances in the codebase that do this shouldn't be doing so.

[asb]

docs/CodingStandards.rst

944

[Sorry, I got confused about who Aaron was responding to].

[asb]

As requested by @majnemer, this updated version of the patch adds in some more explanation about re-evaluating end() and mutation.

[lattner]

FWIW, I agree with @majnemer's comments here: use of auto in those places doesn't aid in understanding/maintainability of the code, and it is important to mention that the manual loop with an explicitly evaluated end can make sense if the sequence is being mutated in the loop (but also we should recommend a comment to make that explicit!)

-Chris

[asb]

Thank you everybody for the feedback. I've made several updates vs the previous version of the patch:

• Don't use auto for the element type in range-based for loops
• Retain most of the previous text on evaluating end() every time for a loop. I initially thought I could relegate this to a shorter note given that explicit iterator-based loops should be relatively rare, but my attempt to do this lost important points.
• Do use auto for the iterator (hopefully this is uncontroversial, as it is explicitly suggested in the "Use `auto` Type Deduction to Make Code More Readable" section

[lattner]

LGTM! Thanks @asb

docs/CodingStandards.rst

Value *doSomething(Instruction *I) {
     ... some long code ....
   }
 
 This fixes these problems.  A similar problem frequently happens in ``for``
 loops.  A silly example is something like this:
 
 .. code-block:: c++
 
-  for (BasicBlock::iterator II = BB->begin(), E = BB->end(); II != E; ++II) {
-    if (BinaryOperator *BO = dyn_cast<BinaryOperator>(II)) {
+  for (auto &I : BB) {
+    if (auto *BO = dyn_cast<BinaryOperator>(&I)) {
       Value *LHS = BO->getOperand(0);
       Value *RHS = BO->getOperand(1);
       if (LHS != RHS) {
         ...
       }
     }
   }
 
 When you have very, very small loops, this sort of structure is fine. But if it
 exceeds more than 10-15 lines, it becomes difficult for people to read and
 understand at a glance. The problem with this sort of code is that it gets very
 nested very quickly. Meaning that the reader of the code has to keep a lot of
 context in their brain to remember what is going immediately on in the loop,
 because they don't know if/when the ``if`` conditions will have ``else``\s etc.
 It is strongly preferred to structure the loop like this:
 
 .. code-block:: c++
 
-  for (BasicBlock::iterator II = BB->begin(), E = BB->end(); II != E; ++II) {
-    BinaryOperator *BO = dyn_cast<BinaryOperator>(II);
+  for (auto &I : BB) {
+    auto *BO = dyn_cast<BinaryOperator>(&I);
     if (!BO) continue;
 
     Value *LHS = BO->getOperand(0);
     Value *RHS = BO->getOperand(1);
     if (LHS == RHS) continue;
 
     ...
   }

 
 A knock-on effect of this stylistic requirement is that when building LLVM with
 GCC you may get warnings related to "control may reach end of non-void function"
 if you return from each case of a covered switch-over-enum because GCC assumes
 that the enum expression may take any representable value, not just those of
 individual enumerators. To suppress this warning, use ``llvm_unreachable`` after
 the switch.
 
-Don't evaluate ``end()`` every time through a loop
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Use range-based ``for`` loops wherever possible
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-Because C++ doesn't have a standard "``foreach``" loop (though it can be
-emulated with macros and may be coming in C++'0x) we end up writing a lot of
-loops that manually iterate from begin to end on a variety of containers or
-through other data structures.  One common mistake is to write a loop in this
-style:
+The introduction of range-based ``for`` loops in C++11 means that explicit
+manipulation of iterators is rarely necessary. We use range-based ``for``
+loops wherever possible for all newly added code. For example:
 
 .. code-block:: c++
 
   BasicBlock *BB = ...
-  for (BasicBlock::iterator I = BB->begin(); I != BB->end(); ++I)
+  for (auto &I : *BB)
     ... use I ...
 
-The problem with this construct is that it evaluates "``BB->end()``" every time
-through the loop.  Instead of writing the loop like this, we strongly prefer
-loops to be written so that they evaluate it once before the loop starts.  A
-convenient way to do this is like so:
+In cases where it is necessary to write an explicit iterator-based loop,
+ensure that `end()` is evaluated only once as long as the container is not
+being mutated. Use code like the following in preference to calling
+``BB->end()`` every time through the loop:
 
 .. code-block:: c++
 
   BasicBlock *BB = ...
   for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I)
     ... use I ...
 
-The observant may quickly point out that these two loops may have different
-semantics: if the container (a basic block in this case) is being mutated, then
-"``BB->end()``" may change its value every time through the loop and the second
-loop may not in fact be correct.  If you actually do depend on this behavior,
-please write the loop in the first form and add a comment indicating that you
-did it intentionally.
-
-Why do we prefer the second form (when correct)?  Writing the loop in the first
-form has two problems. First it may be less efficient than evaluating it at the
-start of the loop.  In this case, the cost is probably minor --- a few extra
-loads every time through the loop.  However, if the base expression is more
-complex, then the cost can rise quickly.  I've seen loops where the end
-expression was actually something like: "``SomeMap[X]->end()``" and map lookups
-really aren't cheap.  By writing it in the second form consistently, you
-eliminate the issue entirely and don't even have to think about it.
-
-The second (even bigger) issue is that writing the loop in the first form hints
-to the reader that the loop is mutating the container (a fact that a comment
-would handily confirm!).  If you write the loop in the second form, it is
-immediately obvious without even looking at the body of the loop that the
-container isn't being modified, which makes it easier to read the code and
-understand what it does.
-
-While the second form of the loop is a few extra keystrokes, we do strongly
-prefer it.
-
 ``#include <iostream>`` is Forbidden
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 The use of ``#include <iostream>`` in library files is hereby **forbidden**,
 because many common implementations transparently inject a `static constructor`_
 into every translation unit that includes it.
   
 Note that using the other stream headers (``<sstream>`` for example) is not