This new patch addresses Chandler's points. On Wed, Jun 6, 2012 at 2:59 PM, Chandler Carruth <[email protected]>wrote:
> On Wed, Jun 6, 2012 at 2:35 PM, Sam Panzer <[email protected]> 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); > >
printf-fixes.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
