[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 ↗

(On Diff #113097)

I'd replace the auto with Instruction &.

964 ↗

(On Diff #113097)

Ditto.

1338 ↗

(On Diff #113097)

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 ↗

(On Diff #113097)

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 ↗

(On Diff #113097)

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 ↗

(On Diff #113097)

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 ↗

(On Diff #113097)

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 ↗

(On Diff #113097)

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 ↗

(On Diff #113097)

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 ↗

(On Diff #113097)

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 ↗

(On Diff #113097)

[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

llvm/trunk/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 (Instruction &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 (Instruction &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.
 
+Use range-based ``for`` loops wherever possible
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+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 (Instruction &I : *BB)
+    ... use I ...
+
 Don't evaluate ``end()`` every time through a loop
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-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:
+In cases where range-based ``for`` loops can't be used and it is necessary
+to write an explicit iterator-based loop, pay close attention to whether
+``end()`` is re-evaluted on each loop iteration. One common mistake is to
+write a loop in this style:
 
 .. code-block:: c++
 
   BasicBlock *BB = ...
-  for (BasicBlock::iterator I = BB->begin(); I != BB->end(); ++I)
+  for (auto I = BB->begin(); I != BB->end(); ++I)
     ... 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:
 
 .. code-block:: c++
 
   BasicBlock *BB = ...
-  for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I)
+  for (auto 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.