Hi Sam, On Thu, Jun 14, 2012 at 3:09 PM, Sam Panzer <[email protected]> wrote: [...] >> >> - TrivialEnough = true; >> >> - } >> >> - } >> >> - } >> >> - >> >> - if (!TrivialEnough && >> >> - getLangOpts().ObjCAutoRefCount && >> >> - E->getType()->isObjCLifetimeType()) >> >> - TrivialEnough = true; >> >> - >> >> - if (TrivialEnough) { >> >> - // Nothing to diagnose. This is okay. >> >> + break; >> >> + case PK_NON_POD: >> >> + if (HasFormatString) { >> >> + // If there is a format string argument, non-POD argument >> >> checking >> >> is >> >> + // handled along with format string checking. >> >> >> >> Does this do the right thing for arguments which aren't format >> >> arguments, >> >> for instance sprintf(my_cstr1, "%d", 1); ? Do we reach the check in the >> >> -Wformat code if the format string is not a constant? >> > >> > >> > Yes, the sprintf() case is handled correctly - that's what >> > getFormatStringInfo() is responsible for. The -Wformat check wasn't >> > reached >> > if the format string wasn't const-qualified, though. This is fixed in >> > the >> > new patch. >> > I added test cases for these two situations to printf-cstr.cpp. >> >> I would imagine you still won't get a diagnostic about the non-pod >> argument in a case like this: >> >> extern const char str[30]; >> printf(str, my_std_string); >> >> I think you need essentially the same set of checks which are >> performed by SemaCheckStringLiteral in order to get this exactly >> right. Perhaps the easiest approach would be to defer all POD checking >> of vararg types to CheckFunctionArguments (after the call to >> SemaCheckStringLiteral) if the callee has a FormatAttr. > > > I factored out the checks from DefaultVariadicArgumentPromotion so that they > are reused right after SemaCheckStringLiteral in CheckFormatArguments as > Richeard suggested. Now any format string arguments that the old warning > would have caught but the format string matching misses should now be > caught, and there's a few extra test to catch this sort of thing.
Great, thanks. I think this is nearly ready now. One more functional change: in the VAK_Invalid case, the argument is transformed from E to (__builtin_trap(), E), but we lose this handling when the function call has a format string. I think you should split up variadicArgumentPODCheck slightly differently, and keep the code to modify the expression in DefaultVariadicArgumentPromotion. The ObjCObjectType check and RequireCompleteType should be moved outside variadicArgumentPODCheck too; we want those errors to be emitted regardless of what might happen with the format string. With that rearrangement done (so variadincArgumentPODCheck just produces the cannot_pass_non_pod_to_vararg warnings), I think it would be straightforward to move the code which generates the non-POD argument diagnostic into SemaChecking's CheckFunctionCall, and return a flag from CheckFormatArguments to indicate if we checked a format string so the diagnostics can be suppressed, rather than calling into the diagnostics code from two different places. There are also a few sets of redundant braces in this patch, which are generally avoided in Clang code. Thanks! Richard _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
