[CPP-434] Clarify Qhelp.

Author: zlaski-semmle

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good1.cpp

@@ -1,3 +1,3 @@
 bool baf(unsigned short n1, unsigned short delta) {
-    return n1 + (unsigned)delta < n1; // GOOD
+    return (unsigned short)(n1 + delta) < n1; // GOOD
 }

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck-good2.cpp

@@ -1,3 +1,3 @@
-bool baz(int n1, unsigned int delta) {
-    return n1 + delta < n1; // GOOD
+bool baz(int n1, int delta) {
+    return (unsigned)n1 + delta < n1; // GOOD
 }

cpp/ql/src/Likely Bugs/Arithmetic/SignedOverflowCheck.qhelp

@@ -4,26 +4,33 @@
 <qhelp>
 <overview>
 <p>
-Testing for signed integer overflow by adding a
-two signed values together and then comparing the result to one
-of the values is ill-formed since the overflow check is undefined.
-The comparison may produce an unintended result, or may be deleted
-by the compiler entirely.
+When checking for integer overflow, one often writes tests like
+<code>a + b &lt; a</code>.  This works fine if <code>a</code> or
+<code>b</code> are unsigned integers, since any overflow in the addition
+will cause the value to simply "wrap around".  However, using
+<i>signed</i> integers is problematic because signed overflows have undefined
+behavior according to the C and C++ standards.  If the addition overflows
+and has an undefined result, the comparison will likewise be undefined;
+it may produce an unintended result, or may be deleted entirely by an
+optimizing compiler.
 </p>
 </overview>
 <recommendation>
 <p>
-When checking for overflow, make sure that <code>unsigned</code> values are used.
+When checking for overflow by adding two values, first make sure that <code>a</code>
+or <code>b</code> are (converted into) unsigned values, unless it is
+certain that the signed addition cannot overflow.
 </p>
 </recommendation>
 <example>
 <p>
 In the following example, even though <code>delta</code> has been declared
 <code>unsigned short</code>, C/C++ type promotion rules require that its
 type is promoted to the larger type used in the addition and comparison,
-namely a <code>signed int</code>.  As a result, the entire expression is
-evaluated using <code>signed</code> integers and may overflow, and hence
-is undefined.
+namely a <code>signed int</code>.  Addition is performed on 
+signed integers, and may have undefined behavior if an overflow occurs.
+As a result, the entire (comparison) expression may also have an undefined
+result.
 </p>
 <sample src="SignedOverflowCheck-bad1.cpp" />
 <p>
@@ -39,21 +46,20 @@ hold true, which likely is not what the programmer intended. (see also the
 <sample src="SignedOverflowCheck-bad2.cpp" />
 <p>
 The following example builds upon the previous one.  Again, we have two
-<code>unsigned short</code> values getting promoted to a wider type.  However,
-since <code>delta</code> is explicitly cast to an <code>unsigned</code> type,
-<code>n1</code> (on both sides of the comparison) is promoted to
-<code>unsigned int</code> as well.  Since we are now operating on
-<code>unsigned</code> values, the overflow check is defined and supported by
-standard C/C++.
+<code>unsigned short</code> values getting promoted to a wider type, resulting
+in a comparison that always succeeds (since there is no overflow).  To
+test whether we have an <code>unsigned short</code> overflow, we cast the
+left-hand side to it, causing the right-hand side to remain an <code>unsigned
+short</code> as well.
 </p>
 <sample src="SignedOverflowCheck-good1.cpp" />
 <p>
-In the next example, a value of type <code>signed int</code> is
-added to a value of type <code>unsigned int</code>.  Because
-the types are of the same size, C/C++ conversion rules dictate that
-<code>unsigned int</code> is chosen as the overall type of the addition
-operation. The entire expression is evaluated using <code>unsigned</code>
-values, which is allowed and defined behavior per the C/C++ standard.
+In the next example, we have two <code>signed int</code> values that we
+wish to add together.  Adding them "as-is" opens the possibility of
+a signed integer overflow, the results of which are undefined.
+By casting one of the operands to <code>unsigned</code>, the entire 
+expression is evaluated using <code>unsigned</code>
+values, which is defined behavior per the C/C++ standard.
 </p>
 <sample src="SignedOverflowCheck-good2.cpp" />
 </example>