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

[David Blaikie]

On Wed, Jun 6, 2012 at 4:56 PM, Sam Panzer <panzer at google.com> wrote:
> This new patch addresses Chandler's points.
>
> On Wed, Jun 6, 2012 at 2:59 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>>
>> 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.
>>
>
> Style nitpicking is important - otherwise I won't learn :)
>
>>
>>
>>
>> +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?
>
>
> Done.
>
>>
>> -
>> +def note_printf_c_str: Note< "did you mean to call .c_str()?">;
>>
>> No space before the "
>
>
> Done.
>
>>
>>
>> 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).
>>
>
> Also done. I was wondering about ObjectiveC strings...
>
>>
>> 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.
>
>
> Done.
>
>>
>>
>> +// 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?
>
>
> It had to do with the offsets of the implicit `this` parameter. It's been
> fixed!
>
>>
>>
>> +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.
>
>
> Done.
>
>>
>>
>> +         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.
>
>
> Unfortunately, that doesn't compile ("member reference type is not a
> pointer"), probably because smallPtrSetIterator doesn't overload operator->.

Hooray, another not-quite-iterator. A perfect opportunity to add an
op-> overload, if you like.

>
>>
>> +
>> +    if (format_idx >= NumArgs) {
>>
>> Avoid curlies on one-line ifs.
>
>
> Done.
>
>>
>>
>> +      // There must be some other format
>> +      continue;
>> +    }
>> +
>> +    return true;
>>
>> Reversing the logic would be shorter:
>>
>> if (format_idx < NumArgs)
>>   return true;
>
>
> Fixed. I shouldn't be missing this sort of thing :)
>
>>
>>
>> +  bool CheckFormatExpr(const analyze_printf::PrintfSpecifier &FS,
>> +                       const char *startSpecifier,
>>
>> Parameter names should be CamelCased. Function (and method) names should
>> be camelCased.
>>
>
> Done, and applied elsewhere in this patch.
>
>>
>> +                       unsigned specifierLen,
>> +                       const Expr *Ex);
>>
>> 'E' is the more common name for an Expr parameter.
>
>
> Done.
>
>>
>>
>> +// 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.
>
>
> Done.
>
>>
>> +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.
>
>
> Done.
>
>>
>>
>> +       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...
>
>
> Parens removed, though operator-> is unavailable here. For future reference,
> is Clang averse to extra variable declarations in favor of extra method
> calls?

I wouldn't really think of op->/op* as function calls for the purpose
of writing code like this. Though as for prevailing style, there are
many loops in Clang/LLVM that follow your style of immediately
declaring a local variable of the iterator's value_type - personally
I'd rather just name the iterator better and use op -> (which, since
the prevailing opinion is that it's OK to have op-> on an iterator
who's value_type is a pointer, means strictly less code - since you're
going to have to use -> on the pointer you declare anyway, and you
could just use it on the iterator directly).

>
>>
>>
>> +    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.
>
>
> Undone.
>
>>
>>
>> +      // This should really be identical to the checks in SemaExpr.cpp.
>>
>> Can you hoist them into a helper function, and call them from here?
>
>
> Done. This required a bit of refactoring in
> DefaultVariadicArgumentPromotion.
>
>>
>>  ExprResult Sema::DefaultVariadicArgumentPromotion(Expr *E,
>> VariadicCallType CT,
>> -                                                  FunctionDecl *FDecl) {
>> +                                                  FunctionDecl *FDecl,
>> +                                                  bool DeferErrors) {
>>
>> DeferErrors doesn't really clarify much for me... maybe "HasFormatString"?
>
>
> Done.
>
>>
>>
>> @@ -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.
>
>
> I'll also start telling diff to ignore whitespace changes.

The opposite, really - you want diff to tell you when you have
whitespace changes so you can avoid committing those whitespace
changes that are unrelated to your actual change.

- David

>
>>
>>
>>      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".
>
>
> Done.
>
>>
>>
>> +    // 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.
>>
>
> True, and done.
>
>>
>> +
>> +    bool DeferErrors = IsPrintfLike(FDecl, Args, NumArgs);
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>