rjmccall added inline comments.

================
Comment at: include/clang/AST/FormatString.h:70
     AsShort,      // 'h'
+    AsInt,        // 'hl' (OpenCL float/int vector element)
     AsLong,       // 'l'
----------------
I think giving this a weird name like `AsShortLong` might help make it clearer 
to readers below that it's a non-standard modifier.


================
Comment at: lib/AST/FormatString.cpp:728
+          return true;
+      }
+
----------------
Your comment here doesn't match the code: you're accepting if this is a FP 
vector, but otherwise you're falling through.  The comment sounds like this 
check should be `if (CS.isDoubleArg()) return !VectorNumElts.isInvalid();`.


================
Comment at: lib/AST/FormatString.cpp:775
+      if (LO.OpenCL && VectorNumElts.isInvalid() && CS.isDoubleArg())
+        return false;
+
----------------
Can you just break the FP args out of the switch below and add this check there?


================
Comment at: lib/Sema/SemaExpr.cpp:755
     }
   }
 
----------------
Can you just do this as a separate patch without the printf checking?  We're 
going to want to merge this to the release branch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57390/new/

https://reviews.llvm.org/D57390



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to