[cfe-commits] [patch] Added a suggestion when a std::string is passed to printf()

[Chandler Carruth]

Some minor comments, and I'll let others start chiming in here to make sure
there is general agreement about the approach:


+def warn_non_pod_vararg_with_format_string : Warning<
+  "cannot pass non-POD object of type %0 to variadic function; "
+  "expected type was %1">,

I'd like to mention that the expectation stems from a format string here:

'... variadic function; the format string expects it to have type %1'

or some such...

+  // Used for determining in which context a type is considered POD
+  enum PODType {
+    POD,
+    CXX11_POD,
+    ObjC_Lifetime_POD,
+    NON_POD
+  };

The LLVM coding conventions on enums are much more precise than other
things. I would vaguely expect it to look like:

enum PODKind {
  PK_NonPOD,
  PK_POD,
  PK_CXX11POD,
  PK_ObjCLifetimePOD
};

+
+  // Determines which PODType fits an expression.
+  PODType levelOfTriviality(const Expr *E);
+

Actually, looking at how this function is implemented, I think you're going
about it the wrong way. This isn't a generalizable concept, it is a very
specific and precise query: isValidTypeForVarargArgument or some such. It
should return true or false, no enum required.

+                                              bool HasformatString =
false);

s/HasformatString/HasFormatString/

+// Given a variadic function, decides whether it looks like we have a
+// printf-style format string and arguments)
+bool Sema::hasFormatStringArgument(FunctionDecl *FDecl,
+                                   Expr **Args, unsigned NumArgs) {

I don't see you using this function from anywhere but new code -- is there
a reason that SemaChecking can't use this logic?

+    // Decide if we're probably inspecting a printf-like function
+
+    bool HasFormatString = hasFormatStringArgument(FDecl, Args, NumArgs);

I think the comment is now extraneous.


On Wed, Jun 6, 2012 at 4:56 PM, Sam Panzer <panzer at google.com> wrote:

> Parens removed, though operator-> is unavailable here. For future
> reference, is Clang averse to extra variable declarations in favor of extra
> method calls?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120606/0a45fbbc/attachment.html>