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

[Alex Bradbury via Phabricator via llvm-commits]

asb updated this revision to Diff 113117.
asb marked an inline comment as done.
asb added a comment.

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


https://reviews.llvm.org/D37264

Files:
  docs/CodingStandards.rst


Index: docs/CodingStandards.rst
===================================================================
--- docs/CodingStandards.rst
+++ docs/CodingStandards.rst
@@ -941,8 +941,8 @@
 
 .. 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) {
@@ -961,8 +961,8 @@
 
 .. 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);
@@ -1322,58 +1322,32 @@
 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, pay
+close attention to whether ``end()`` is re-evaluted on each loop iteration. If
+the container is not mutated, the loop should be written as below so ``end()``
+is evaluated only once. As well as being a minor performance optimisation,
+this style makes it clear when reading the code that the container isn't being
+modified during 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
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D37264.113117.patch
Type: text/x-patch
Size: 4313 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170829/38a59183/attachment.bin>