This new patch addresses Chandler's points.<br><br><div class="gmail_quote">On Wed, Jun 6, 2012 at 2:59 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div class="im">On Wed, Jun 6, 2012 at 2:35 PM, Sam Panzer <span dir="ltr"><<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.</blockquote><div><br></div></div><div>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.</div>
<div><br></div></div></blockquote><div><br></div><div>Style nitpicking is important - otherwise I won't learn :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div></div><div><br></div><div><div></div><div><br></div><div>+def warn_non_pod_string_passed_to_printf : Warning<</div><div><br></div><div>This warning name is a bit misleading now... How about: warn_non_pod_vararg_with_format_string?</div>
</div></div></blockquote><div> </div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>
<div>- </div><div>+def note_printf_c_str: Note< "did you mean to call .c_str()?">;</div><div><br></div><div>No space before the "</div></div></div></blockquote><div><br></div><div>Done.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div><br></div><div>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).</div>
<div><br></div></div></div></blockquote><div><br></div><div>Also done. I was wondering about ObjectiveC strings...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div></div><div>I would probably use more of a prose form rather than potentially getting the syntax wrong: "did you mean to call the %0 method?"</div><div><br></div><div><br></div>
<div>+ bool IsPrintfLike(FunctionDecl *FDecl, Expr **Args, unsigned NumArgs);</div>
<div><br></div><div>I would prefer 'hasFormatStringArgument'. I think that's more accurate.</div></div></div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div><br></div><div>+// Given a variadic function, decides whether it looks like we have a</div><div>+// printf-style format string and arguments)</div>
<div>+// This doesn't work for member functions.</div><div><br></div><div>Why not?</div></div></div></blockquote><div><br></div><div>It had to do with the offsets of the implicit `this` parameter. It's been fixed!</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div><br></div><div>+bool Sema::IsPrintfLike(FunctionDecl *FDecl, Expr **Args, unsigned NumArgs) {</div>
<div>+ // Anonymous and lambda functions will be assumed not to be printf.</div>
<div>+ if (!FDecl)</div><div>+ return false;</div><div>+</div><div>+ for (specific_attr_iterator<FormatAttr></div><div>+ i = FDecl->specific_attr_begin<FormatAttr>(),</div><div><br></div><div>
Prevailing style you should use in new patches is for iterator loop variables to use capital letters: I and E.</div></div></div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>
<div><br></div><div>+ e = FDecl->specific_attr_end<FormatAttr>(); i != e ; ++i) {</div><div>+ const FormatAttr *Format = *i;</div><div>+ unsigned format_idx = Format->getFormatIdx() - 1;</div><div>
<br></div><div>I think you can just use 'I->' here.</div></div></div></blockquote><div><br></div><div>Unfortunately, that doesn't compile ("member reference type is not a pointer"), probably because smallPtrSetIterator doesn't overload operator->.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div><br></div><div>+</div><div>+ if (format_idx >= NumArgs) {</div>
<div><br></div><div>Avoid curlies on one-line ifs.</div></div></div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div><br></div><div>+ // There must be some other format</div>
<div>+ continue;</div><div>+ }</div><div>+</div><div>+ return true;</div><div><br></div><div>Reversing the logic would be shorter:</div><div><br></div><div>if (format_idx < NumArgs)</div><div> return true;</div>
</div></div></blockquote><div><br></div><div>Fixed. I shouldn't be missing this sort of thing :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">
<div>
<div><br></div><div>+ bool CheckFormatExpr(const analyze_printf::PrintfSpecifier &FS,</div><div>+ const char *startSpecifier,</div><div><br></div><div>Parameter names should be CamelCased. Function (and method) names should be camelCased.</div>
<div><br></div></div></div></blockquote><div><br></div><div>Done, and applied elsewhere in this patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div></div><div>+ unsigned specifierLen,</div><div>+ const Expr *Ex);</div><div><br></div><div>'E' is the more common name for an Expr parameter.</div>
</div></div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div><br></div>
<div>+// Determines if the specified is a C++ class or struct containing</div><div>+// a member with the specified name and kind (e.g. a CXXMethodDecl named</div><div>+// "c_str()"</div><div>+template<typename FieldKind></div>
<div>+static llvm::SmallPtrSet<FieldKind*, 1></div><div>+CXXRecordMembersNamed(StringRef Name, Sema &S, QualType Ty) {</div><div>+ const RecordType *RT = Ty->getAs<RecordType>();</div><div>+ llvm::SmallPtrSet<FieldKind*, 1> Results;</div>
<div>+</div><div>+ if (!RT)</div><div>+ return Results;</div><div>+ const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(RT->getDecl());</div><div>+ if (!RD)</div><div>+ return Results;</div><div>+</div><div>
+ LookupResult R(S, &S.PP.getIdentifierTable().get(Name), SourceLocation(),</div><div>+ Sema::LookupMemberName);</div><div>+</div><div>+ // We just need to include all members of the right kind turned up by the</div>
<div>+ // filter, at this point.</div><div>+ if (S.LookupQualifiedName(R, RT->getDecl())) {</div><div>+ LookupResult::Filter filter = R.makeFilter();</div><div>+ while (filter.hasNext()) {</div><div>+ NamedDecl *decl = filter.next()->getUnderlyingDecl();</div>
<div>+ if (FieldKind *FK = dyn_cast<FieldKind>(decl)) {</div><div>+ Results.insert(FK);</div><div>+ }</div><div>+ }</div><div>+ filter.done();</div><div>+ }</div><div>+ return Results;</div>
<div>+}</div><div>+</div><div>+// Check if a (w)string was passed when a (w)char* was needed, and offer a</div><div>+// better diagnostic if so. ATR is assumed to be valid.</div><div>+// Returns true when a c_str() conversion method is found.</div>
<div>+bool</div><div><br></div><div>Please try not to break after return types unless you have to for 80-columns.</div></div></div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div></div><div>+CheckPrintfHandler::CheckForCStrMembers(</div><div>+ const analyze_printf::ArgTypeResult &ATR, const Expr *Ex,</div>
<div>+ const CharSourceRange &CSR) {</div><div>+ typedef llvm::SmallPtrSet<CXXMethodDecl*, 1> MethodSet;</div><div>+ MethodSet Results =</div><div>+ CXXRecordMembersNamed<CXXMethodDecl>("c_str", S, Ex->getType());</div>
<div>+</div><div>+ for (MethodSet::iterator it = Results.begin(), end = Results.end();</div><div><br></div><div>I and E here rather than it and end.</div></div></div></blockquote><div><br></div><div>Done.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div><br></div><div>+ it != end; ++it) {</div><div>+ const CXXMethodDecl *Method = (*it);</div>
<div><br></div><div>Don't use extraneous parentheses please. Also, I suspect you can directly use 'I->' below rather than creating a separate variable...</div></div></div></blockquote><div><br></div><div>Parens removed, though operator-> is unavailable here. For future reference, is Clang averse to extra variable declarations in favor of extra method calls? </div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div><br></div><div>+ if (Method->getNumParams() == 0 &&</div>
<div>+ ATR.matchesType(S.Context, Method->getResultType())) {</div><div>+</div><div>+ S.Diag(Ex->getLocStart(), diag::note_printf_c_str)</div><div>+ << FixItHint::CreateInsertion(Ex->getLocEnd(), ".c_str()");</div>
<div>+ return true;</div><div>+ }</div><div>+ }</div><div>+</div><div>+ return false;</div><div>+}</div><div>+</div><div> bool</div><div> CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier</div>
<div> &FS,</div><div> const char *startSpecifier,</div><div> unsigned specifierLen) {</div>
<div>-</div><div><br></div><div>Please don't change unrelated whitespace.</div></div></div></blockquote><div><br></div><div>Undone.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div><br></div><div>+ // This should really be identical to the checks in SemaExpr.cpp.</div><div><br></div><div>Can you hoist them into a helper function, and call them from here?</div>
</div></div></blockquote><div><br></div><div>Done. This required a bit of refactoring in DefaultVariadicArgumentPromotion.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>
<div><br></div><div> ExprResult Sema::DefaultVariadicArgumentPromotion(Expr *E, VariadicCallType CT,</div><div>- FunctionDecl *FDecl) {</div><div>+ FunctionDecl *FDecl,</div>
<div>+ bool DeferErrors) {</div><div><br></div><div>DeferErrors doesn't really clarify much for me... maybe "HasFormatString"?</div></div></div></blockquote>
<div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div><div><br></div><div>@@ -585,9 +586,14 @@ ExprResult Sema::DefaultVariadicArgumentPromotion(Expr *E, VariadicCallType CT,</div>
<div> getLangOpts().ObjCAutoRefCount &&</div><div> E->getType()->isObjCLifetimeType())</div><div> TrivialEnough = true;</div><div>- </div><div>+</div><div><br></div><div>You'll probably have to fix your editor to avoid unrelated whitespace changes.</div>
</div></div></blockquote><div><br></div><div>I'll also start telling diff to ignore whitespace changes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>
<div><br></div><div> if (TrivialEnough) {</div><div> // Nothing to diagnose. This is okay.</div><div>+ } else if (DeferErrors) {</div><div>+ // In case the function looks sufficiently like printf,</div>
<div>
+ // try to fix non-POD arguments (e.g. an std::string passed rather than</div><div>+ // a char *).</div><div>+ // This is handled in SemaChecking, so we skip the error here.</div><div><br></div><div>I just would say: "If there is a format string argument, non-POD argument checking is handled along with the format string checking".</div>
</div></div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>
<div><br></div><div>+ // Decide if we're probably inspecting a printf-like function</div><div><br></div><div>I'm hopeful that with the function and variable name changes you can drop this comment. It should be obvious from the code.</div>
<div><br></div></div></div></blockquote><div><br></div><div>True, and done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">
<div><div></div><div>+</div><div>+ bool DeferErrors = IsPrintfLike(FDecl, Args, NumArgs);</div><div><br></div></div></div>
</blockquote></div><br>