<div class="gmail_quote">Just nit-picking a bit...</div><div class="gmail_quote"><br></div><div class="gmail_quote">On Thu, Sep 1, 2011 at 7:15 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div id=":7hi">+/// \brief Warn if Operand is incomplete pointer type<br>
+///<br>
+/// \returns True if pointer has incomplete type<br>
+static bool checkArithmeticIncompletePointerType(Sema &S, SourceLocation Loc,<br>
+                                                 Expr *Operand) {<br>
+  if ((Operand->getType()->isPointerType() &&<br>
+       !Operand->getType()->isDependentType()) ||<br>
+      Operand->getType()->isObjCObjectPointerType()) {<br>
+    QualType PointeeTy = Operand->getType()->getPointeeType();<br>
+    if (S.RequireCompleteType(<br>
+          Loc, PointeeTy,<br>
+          S.PDiag(diag::err_typecheck_arithmetic_incomplete_type)<br>
+            << PointeeTy << Operand->getSourceRange()))<br></div></blockquote><div><br></div><div>This if makes my eyes hurt. Maybe put the partial diagnostic in a variable, and pass that in? Would likely make the whole thing much easier to read.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div id=":7hi">
+      return true;<br>
+  }<br>
+  return false;<br>
+}<br>
+<br>
 /// \brief Check the validity of an arithmetic pointer operand.<br>
 ///<br>
 /// If the operand has pointer type, this code will check for pointer types<br>
@@ -5815,16 +5833,7 @@<br>
     return !S.getLangOptions().CPlusPlus;<br>
   }<br>
<br>
-  if ((Operand->getType()->isPointerType() &&<br>
-       !Operand->getType()->isDependentType()) ||<br>
-      Operand->getType()->isObjCObjectPointerType()) {<br>
-    QualType PointeeTy = Operand->getType()->getPointeeType();<br>
-    if (S.RequireCompleteType(<br>
-          Loc, PointeeTy,<br>
-          S.PDiag(diag::err_typecheck_arithmetic_incomplete_type)<br>
-            << PointeeTy << Operand->getSourceRange()))<br>
-      return false;<br>
-  }<br>
+  if (checkArithmeticIncompletePointerType(S, Loc, Operand)) return false;<br></div></blockquote><div><br></div><div>Here and below, I'd probably put the 'return false;' on its own line so its easier to spot when scanning.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div id=":7hi">
<br>
   return true;<br>
 }<br>
@@ -5869,20 +5878,9 @@<br>
     return !S.getLangOptions().CPlusPlus;<br>
   }<br>
<br>
-  Expr *Operands[] = { LHS, RHS };<br>
-  for (unsigned i = 0; i < 2; ++i) {<br>
-    Expr *Operand = Operands[i];<br>
-    if ((Operand->getType()->isPointerType() &&<br>
-         !Operand->getType()->isDependentType()) ||<br>
-        Operand->getType()->isObjCObjectPointerType()) {<br>
-      QualType PointeeTy = Operand->getType()->getPointeeType();<br>
-      if (S.RequireCompleteType(<br>
-            Loc, PointeeTy,<br>
-            S.PDiag(diag::err_typecheck_arithmetic_incomplete_type)<br>
-              << PointeeTy << Operand->getSourceRange()))<br>
-        return false;<br>
-    }<br>
-  }<br>
+  if (checkArithmeticIncompletePointerType(S, Loc, LHS)) return false;<br>
+  if (checkArithmeticIncompletePointerType(S, Loc, RHS)) return false;<br>
+</div></blockquote></div><br>