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

[Alex Bradbury via Phabricator via llvm-commits]

asb updated this revision to Diff 113245.
asb marked 10 inline comments as done.
asb edited the summary of this revision.
asb added a comment.

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


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 (Instruction &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 (Instruction &I : BB) {
+    auto *BO = dyn_cast<BinaryOperator>(&I);
     if (!BO) continue;
 
     Value *LHS = BO->getOperand(0);
@@ -1322,19 +1322,31 @@
 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
@@ -1345,7 +1357,7 @@
 .. 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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D37264.113245.patch
Type: text/x-patch
Size: 2630 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170830/3bc54c02/attachment-0001.bin>