[PATCH] Add limitations to loop convert user doc

[Jack Yang]

  Addressed review comments. Thanks!

  @silvas, regarding the "Iterator" subheading: My intention for that section was to talk about how the range-based for loop calls end() only once during the initialization of the loop, so if the original header called end() after each iteration, semantics may change. It was not meant to be limited to incorrect transformations at the risk=risky level.
  I've moved the link of the risky example to the actual example and not the section, but suggestions to make it clearer are welcome. Thanks.

Hi revane, silvas, gribozavr,

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

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D552?vs=1316&id=1335#toc

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,10 @@
   for (int i = 0; i < obj.getVector().size(); ++i)
     obj.foo(10); // using 'obj' is considered risky
 
+See :ref:`Iterator Limitations<IncorrectRiskyTransformation>` for an example of
+an incorrect transformation when the maximum acceptable risk level is set to
+`risky`.
+
 Reasonable (Default)
 ^^^^^^^^^^^^^^^^^^^^
 
@@ -118,3 +119,139 @@
   for (auto & elem : v)
     cout << elem;
 
+Limitations
+===========
+
+There are certain situations where the tool may erroneously perform
+transformations that removes information and changes semantics. The following
+is a list of known limitations of the transform. Users of the tool should be
+aware of the behaviour and limitations of the transform.
+
+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) { }
+
+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 may differ.
+
+.. code-block:: c++
+
+  // Instead of calling .end() after each iteration, this loop will be
+  // transformed to call .end() only once during the initialization of the loop,
+  // which may 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) { }
+
+.. _IncorrectRiskyTransformation:
+
+As explained previously, calling member functions of 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 ``.end()``
+only being called at the start of the loop.
+
+.. code-block:: c++
+
+  bool flag = false;
+  for (vector<T>::iterator it = vec.begin(); it != vec.end(); ++it) {
+    // Add a copy of the first element to the end of the vector.
+    if (!flag) {
+      // This line makes this transformation 'risky'.
+      vec.push_back(*it);
+      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) {
+    // Add a copy of the first element to the end of the vector.
+    if (!flag) {
+      // This line makes this transformation 'risky'
+      vec.push_back(elem);
+      flag = true;
+    }
+    cout << elem;
+  }
+
+Semantics may also be affected if ``.end()`` has side effects. For example, in
+the case where calls to ``.end()`` are logged, the semantics may change in the
+transformed loop if ``.end()`` was called after each iteration.
+
+.. code-block:: c++
+
+  iterator end() {
+    num_of_end_calls++;
+    return container.end();
+  }
+
+Overloaded Arrow Operator
+^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Similarly, if ``operator->()`` was overloaded to have side effects, such as
+logging, the semantics will change. In the original loop, we may have used the
+arrow operator to dereference the iterator to call a member. This is no longer
+necessary in the transformed loop, since we will replace it to use the dot
+operator. Therefore any side effect of the arrow operator will no longer be
+performed.
+
+.. code-block:: c++
+
+  for (iterator it = c.begin(); it != c.end(); ++it) {
+    it->func(); // Using operator->()
+  }
+  // Will be transformed to:
+  for (auto & elem : c) {
+    elem.func(); // No longer using operator->()
+  }
+
+
+Pointers and References
+^^^^^^^^^^^^^^^^^^^^^^^
+
+While most of the transform'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.
+
+If the container were directly used instead of using the pointer or reference,
+the following transformation would have only been applied at the ``-risk=risky``
+level, since calling a member function of the container is considered `risky`.
+Loop convert cannot identify expressions associated with the container that are
+different than the one used in the loop header, therefore the transformation
+below ends up being performed at the ``-risk=safe`` 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.2.patch
Type: text/x-patch
Size: 5386 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130320/2c7eee6f/attachment.bin>