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

[Chandler Carruth]

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

> And here is another update that replaces the default non-POD error with a
> smarter (printf-specific) one, rather than just tacking on an extra warning.


Thanks, I really love this. =] Comments on the patch. A lot of this is just
style nit-picking to help you get used to the coding conventions in Clang.



+def warn_non_pod_string_passed_to_printf : Warning<

This warning name is a bit misleading now... How about:
warn_non_pod_vararg_with_format_string?
-
+def note_printf_c_str: Note< "did you mean to call .c_str()?">;

No space before the "

Also, you should probably pass 'c_str' from the name of the method so that
we can generalize this later if we would like (str, dunno, maybe some
interesting suggestions for ObjectiveC types).

I would probably use more of a prose form rather than potentially getting
the syntax wrong: "did you mean to call the %0 method?"


+  bool IsPrintfLike(FunctionDecl *FDecl, Expr **Args, unsigned NumArgs);

I would prefer 'hasFormatStringArgument'. I think that's more accurate.

+// Given a variadic function, decides whether it looks like we have a
+// printf-style format string and arguments)
+// This doesn't work for member functions.

Why not?

+bool Sema::IsPrintfLike(FunctionDecl *FDecl, Expr **Args, unsigned
NumArgs) {
+  // Anonymous and lambda functions will be assumed not to be printf.
+  if (!FDecl)
+    return false;
+
+  for (specific_attr_iterator<FormatAttr>
+         i = FDecl->specific_attr_begin<FormatAttr>(),

Prevailing style you should use in new patches is for iterator loop
variables to use capital letters: I and E.

+         e = FDecl->specific_attr_end<FormatAttr>(); i != e ; ++i) {
+    const FormatAttr *Format = *i;
+    unsigned format_idx = Format->getFormatIdx() - 1;

I think you can just use 'I->' here.

+
+    if (format_idx >= NumArgs) {

Avoid curlies on one-line ifs.

+      // There must be some other format
+      continue;
+    }
+
+    return true;

Reversing the logic would be shorter:

if (format_idx < NumArgs)
  return true;

+  bool CheckFormatExpr(const analyze_printf::PrintfSpecifier &FS,
+                       const char *startSpecifier,

Parameter names should be CamelCased. Function (and method) names should be
camelCased.

+                       unsigned specifierLen,
+                       const Expr *Ex);

'E' is the more common name for an Expr parameter.

+// Determines if the specified is a C++ class or struct containing
+// a member with the specified name and kind (e.g. a CXXMethodDecl named
+// "c_str()"
+template<typename FieldKind>
+static llvm::SmallPtrSet<FieldKind*, 1>
+CXXRecordMembersNamed(StringRef Name, Sema &S, QualType Ty) {
+  const RecordType *RT = Ty->getAs<RecordType>();
+  llvm::SmallPtrSet<FieldKind*, 1> Results;
+
+  if (!RT)
+    return Results;
+  const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(RT->getDecl());
+  if (!RD)
+    return Results;
+
+  LookupResult R(S, &S.PP.getIdentifierTable().get(Name), SourceLocation(),
+                 Sema::LookupMemberName);
+
+  // We just need to include all members of the right kind turned up by the
+  // filter, at this point.
+  if (S.LookupQualifiedName(R, RT->getDecl())) {
+    LookupResult::Filter filter = R.makeFilter();
+    while (filter.hasNext()) {
+      NamedDecl *decl = filter.next()->getUnderlyingDecl();
+      if (FieldKind *FK = dyn_cast<FieldKind>(decl)) {
+        Results.insert(FK);
+      }
+    }
+    filter.done();
+  }
+  return Results;
+}
+
+// Check if a (w)string was passed when a (w)char* was needed, and offer a
+// better diagnostic if so. ATR is assumed to be valid.
+// Returns true when a c_str() conversion method is found.
+bool

Please try not to break after return types unless you have to for
80-columns.

+CheckPrintfHandler::CheckForCStrMembers(
+    const analyze_printf::ArgTypeResult &ATR, const Expr *Ex,
+    const CharSourceRange &CSR) {
+  typedef llvm::SmallPtrSet<CXXMethodDecl*, 1> MethodSet;
+  MethodSet Results =
+      CXXRecordMembersNamed<CXXMethodDecl>("c_str", S, Ex->getType());
+
+  for (MethodSet::iterator it = Results.begin(), end = Results.end();

I and E here rather than it and end.

+       it != end; ++it) {
+    const CXXMethodDecl *Method = (*it);

Don't use extraneous parentheses please. Also, I suspect you can directly
use 'I->' below rather than creating a separate variable...

+    if (Method->getNumParams() == 0 &&
+          ATR.matchesType(S.Context, Method->getResultType())) {
+
+      S.Diag(Ex->getLocStart(), diag::note_printf_c_str)
+          << FixItHint::CreateInsertion(Ex->getLocEnd(), ".c_str()");
+      return true;
+    }
+  }
+
+  return false;
+}
+
 bool
 CheckPrintfHandler::HandlePrintfSpecifier(const
analyze_printf::PrintfSpecifier
                                             &FS,
                                           const char *startSpecifier,
                                           unsigned specifierLen) {
-

Please don't change unrelated whitespace.

+      // This should really be identical to the checks in SemaExpr.cpp.

Can you hoist them into a helper function, and call them from here?

 ExprResult Sema::DefaultVariadicArgumentPromotion(Expr *E,
VariadicCallType CT,
-                                                  FunctionDecl *FDecl) {
+                                                  FunctionDecl *FDecl,
+                                                  bool DeferErrors) {

DeferErrors doesn't really clarify much for me... maybe "HasFormatString"?

@@ -585,9 +586,14 @@ ExprResult Sema::DefaultVariadicArgumentPromotion(Expr
*E, VariadicCallType CT,
         getLangOpts().ObjCAutoRefCount &&
         E->getType()->isObjCLifetimeType())
       TrivialEnough = true;
-
+

You'll probably have to fix your editor to avoid unrelated whitespace
changes.

     if (TrivialEnough) {
       // Nothing to diagnose. This is okay.
+    } else if (DeferErrors) {
+      // In case the function looks sufficiently like printf,
+      // try to fix non-POD arguments (e.g. an std::string passed rather
than
+      // a char *).
+      // This is handled in SemaChecking, so we skip the error here.

I just would say: "If there is a format string argument, non-POD argument
checking is handled along with the format string checking".

+    // Decide if we're probably inspecting a printf-like function

I'm hopeful that with the function and variable name changes you can drop
this comment. It should be obvious from the code.

+
+    bool DeferErrors = IsPrintfLike(FDecl, Args, NumArgs);
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120606/6c2160e1/attachment.html>