<div style="font-family: arial, helvetica, sans-serif"><font size="2">Ping.<br><br><div class="gmail_quote">On Wed, Jun 6, 2012 at 6: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"><div class="gmail_quote"><div>Thanks again for the input.</div><div><br></div><div>Of the two main issues,</div><div>(1) the POD_Type enum<br>
I used the enum to distinguish between three cases: one requires a warning for C++98 compatibility, one requires the standard can't-pass-non-pod-to-vararg warning, and the third does nothing. Since the code that checks for non-POD arguments to vararg functions lives in two places because of the way format-string checking is implemented, I thought it made sense to separate the pod-kind-determining logic from the warning-emitting logic.</div>
<div><br></div><div>Does someone have a suggestion for implementing this? I can think of two other approaches, and neither seems as clean: levelOfTriviality could either take an extra argument that tells it not to emit warnings, or it could take a series of callbacks for the three cases.</div>
<div><br></div><div>(2) It is possible for SemaChecking to reuse the logic from hasFormatStringArgument(), since both of them really compute the (expected) location of the format string. I refactored a bit to share the relevant logic, though there isn't too much in common. Is this an improvement?</div>
</div><div class="HOEnZb"><div class="h5"><div class="gmail_quote"><br></div><div class="gmail_quote">On Wed, Jun 6, 2012 at 5:20 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">Some minor comments, and I'll let others start chiming in here to make sure there is general agreement about the approach:<div>
<br></div><div><br></div><div><div>+def warn_non_pod_vararg_with_format_string : Warning<</div>
<div>+ "cannot pass non-POD object of type %0 to variadic function; "</div><div>+ "expected type was %1">,</div></div><div><br></div><div>I'd like to mention that the expectation stems from a format string here:</div>
<div><br></div><div>'... variadic function; the format string expects it to have type %1'</div><div><br></div><div>or some such...</div><div><br></div><div><div>+ // Used for determining in which context a type is considered POD</div>
<div>+ enum PODType {</div><div>+ POD,</div><div>+ CXX11_POD,</div><div>+ ObjC_Lifetime_POD,</div><div>+ NON_POD</div><div>+ };</div><div><br></div><div>The LLVM coding conventions on enums are much more precise than other things. I would vaguely expect it to look like:</div>
<div><br></div><div>enum PODKind {</div><div> PK_NonPOD,</div><div> PK_POD,</div><div> PK_CXX11POD,</div><div> PK_ObjCLifetimePOD</div><div>};</div><div><br></div><div>+</div><div>+ // Determines which PODType fits an expression.</div>
<div>+ PODType levelOfTriviality(const Expr *E);</div><div>+</div><div><br></div><div>Actually, looking at how this function is implemented, I think you're going about it the wrong way. This isn't a generalizable concept, it is a very specific and precise query: isValidTypeForVarargArgument or some such. It should return true or false, no enum required.</div>
</div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<div><br></div><div><div>+ bool HasformatString = false);</div></div><div><br></div><div>s/HasformatString/HasFormatString/</div><div><br></div><div><div><div>+// Given a variadic function, decides whether it looks like we have a</div>
<div>+// printf-style format string and arguments)</div></div><div>+bool Sema::hasFormatStringArgument(FunctionDecl *FDecl,</div><div><div>+ Expr **Args, unsigned NumArgs) {</div>
</div></div><div><br></div>
<div>I don't see you using this function from anywhere but new code -- is there a reason that SemaChecking can't use this logic?</div><div><br></div><div><div><div>+ // Decide if we're probably inspecting a printf-like function</div>
</div><div>+</div><div>+ bool HasFormatString = hasFormatStringArgument(FDecl, Args, NumArgs);</div></div><div><br></div><div>I think the comment is now extraneous.</div><div><div><br></div><br><div class="gmail_quote">
On Wed, Jun 6, 2012 at 4:56 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">Parens removed, though operator-> is unavailable here. For future reference, is Clang averse to extra variable declarations in favor of extra method calls? </blockquote>
</div><br></div></div>
</blockquote></div><br>
</div></div></blockquote></div><br></font></div>