[PATCH] Add limitations to loop convert user doc

[Jack Yang]

Hi revane, silvas, gribozavr,

Sam Panzer, author of loop convert, provided a list of limitations of
the tool to be documented. (Thanks Sam!) These limitations are now
added to the loop convert user's documentation. Included are examples
of situations where the tool may change semantics, including examples
of incorrect transformations applied when the risk level is set too
high.

http://llvm-reviews.chandlerc.com/D552

Files:
  docs/LoopConvertTransform.rst

Index: docs/LoopConvertTransform.rst
===================================================================
--- docs/LoopConvertTransform.rst
+++ docs/LoopConvertTransform.rst
@@ -17,9 +17,6 @@
 Risk
 ====
 
-TODO: Add code examples for which incorrect transformations are performed
-when the risk level is set to "Risky" or "Reasonable".
-
 Risky
 ^^^^^
 
@@ -41,6 +38,9 @@
   for (int i = 0; i < obj.getVector().size(); ++i)
     obj.foo(10); // using 'obj' is considered risky
 
+See :ref:`IteratorLimitations` for an example of an incorrect transformation
+when the maximum acceptable risk level is set to `risky`.
+
 Reasonable (Default)
 ^^^^^^^^^^^^^^^^^^^^
 
@@ -51,7 +51,9 @@
 
 .. code-block:: c++
 
-  // using size() is considered reasonable
+  for (iterator it = container.begin(), e = container.end(); it != e; it++)
+    cout << *it;
+
   for (int i = 0; i < container.size(); ++i)
     cout << container[i];
 
@@ -69,6 +71,9 @@
   for (int i = 0; i < 3; ++i)
     cout << arr[i];
 
+  for (iterator it = container.begin(), e = container.end(); it != e; it++)
+    cout << *it;
+
 Example
 =======
 
@@ -87,9 +92,13 @@
   for (int i = 0; i < N; ++i)
     cout << arr[i];
 
+  // safe transform
+  for (vector<int>::iterator it = v.begin(), e = v.end(); it != e; ++it)
+    cout << *it;
+
   // reasonable transform
   for (vector<int>::iterator it = v.begin(); it != v.end(); ++it)
-    cout << *it;*
+    cout << *it;
 
   // reasonable transform
   for (int i = 0; i < v.size(); ++i)
@@ -110,11 +119,122 @@
   for (auto & elem : arr)
     cout << elem;
 
+  // safe transform
+  for (auto & elem : v)
+    cout << elem;
+
   // reasonable transform
   for (auto & elem : v)
     cout << elem;
 
   // reasonable transform
   for (auto & elem : v)
     cout << elem;
 
+Limitations
+===========
+
+Comments
+^^^^^^^^
+
+Comments inside the original loop header are ignored and deleted when
+transformed.
+
+.. code-block:: c++
+
+  for (int i = 0; i < N; /* This will be deleted */ ++i) { }
+
+.. _IteratorLimitations:
+
+Iterators
+^^^^^^^^^
+
+The C++11 range-based for loop calls ``.end()`` only once during the
+initialization of the loop. If in the original loop, ``.end()`` is called after
+each iteration, the semantics of the transformed loop will differ.
+
+.. code-block:: c++
+
+  // Instead of calling .end() after each iteration, it will be transformed
+  // to call it only once during the initialization of the loop, which will
+  // affect semantics.
+  for (iterator it = container.begin(); it != container.end(); ++it) { }
+
+  // The following is semantically equivalent to the C++11 range-based for loop,
+  // therefore the semantics of the header will not change.
+  for (iterator it = container.begin(), e = container.end(); it != e; ++it) { }
+
+As explained previously, calling the container in the body of the loop is
+considered `risky`. If the called member function modifies the container, the
+semantics of the converted loop will differ due to this iterator limitation.
+
+.. code-block:: c++
+
+  bool flag = false;
+  for (vector<T>::iterator it = vec.begin(); it != vec.end(); ++it) {
+    if (!flag) { // Add a copy of the first element to the end of the vector
+      vec.push_back(*it); // This line makes this transformation 'risky'
+      flag = true;
+    }
+    cout << *it;
+  }
+
+The original code above prints out the contents of the container including the
+newly added element, while the converted loop, shown below, will only print the
+original contents and not the newly added element.
+
+.. code-block:: c++
+
+  bool flag = false;
+  for (auto & elem : vec) {
+    if (!flag) { // Add a copy of the first element to the end of the vector
+      vec.push_back(elem); // This line makes this transformation 'risky'
+      flag = true;
+    }
+    cout << elem;
+  }
+
+This limitation may also arise if each call to ``.end()`` is logged. If
+``.end()`` is called after each iteration in the original loop, and if it does
+something in addition to returning the iterator, the semantics will change.
+
+.. code-block:: c++
+
+  iterator end() {
+    num_of_end_calls++;
+    return container.end();
+  }
+
+Similarly, if each dereference operator is logged, the semantics of the
+transformed loop will change. Since the transformed loop no longer needs to
+dereference an iterator, it may cause changes in semantics if ``operator*()``
+is overloaded.
+
+Pointers and References
+^^^^^^^^^^^^^^^^^^^^^^^
+
+While most of loop convert's risk analysis is dedicated to determining whether
+the iterator or container was modified, it is possible to circumvent the
+analysis by accessing and modifying the container through a pointer or
+reference.
+
+The following transformation is performed at the ``-risk=safe`` level. If we had
+directly used the container instead of the pointer or reference, the
+transformation would have only been applied at the ``-risk=risky`` level.
+
+.. code-block:: c++
+
+  vector<int> vec;
+
+  vector<int> *ptr = &vec;
+  vector<int> &ref = vec;
+
+  for (vector<int>::iterator it = vec.begin(), e = vec.end(); it != e; ++it) {
+    if (!flag) {
+      // Accessing and modifying the container is considered risky, but the risk
+      // level is not raised here.
+      ptr->push_back(*it);
+      ref.push_back(*it);
+      flag = true;
+    }
+  }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D552.1.patch
Type: text/x-patch
Size: 5340 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130319/22805fad/attachment.bin>