[cfe-commits] [patch] Added a suggestion when a std::string is passed to printf()
Sam Panzer
panzer at google.com
Wed Jun 6 16:56:30 PDT 2012
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->.
> +
> + 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?
>
> + 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.
>
> 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);
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120606/2725ab27/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: printf-fixes.patch
Type: application/octet-stream
Size: 20502 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120606/2725ab27/attachment.obj>
More information about the cfe-commits
mailing list