[cfe-commits] [Patch] -Wformat: properly handle length modifiers used with %n (Was: -Wformat: warn about using length modifiers with %n)

[Ted Kremenek]

This looks great.  One comment:

>  std::string ArgType::getRepresentativeTypeName(ASTContext &C) const {
>    std::string S = getRepresentativeType(C).getAsString();
> -  if (Name && S != Name)
> -    return std::string("'") + Name + "' (aka '" + S + "')";
> +  std::string Alias;
> +  if (Name) {
> +    Alias = Name;
> +    if (Ptr)
> +      Alias += (Alias[Alias.size()-1] == '*') ? "*" : " *";
> +    if (S == Alias)
> +      Alias.clear();
> +  }
> +
> +  if (!Alias.empty())
> +    return std::string("'") + Alias + "' (aka '" + S + "')";
>    return std::string("'") + S + "'";
>  }

Can you add some comments here, specifically around the string construction, that says what it is trying to accomplish?

Overall, the rest of it looks pretty obvious.  Nice clean refactoring, with a bunch of the expected checking logic.

On Aug 4, 2012, at 12:09 PM, Hans Wennborg <hans at chromium.org> wrote:

> On Thu, Aug 2, 2012 at 6:31 PM, Ted Kremenek <kremenek at apple.com> wrote:
>> C99 says you can use %n with the length modifiers 'hh', 'h', 'l', 'j', 'z', and 't'.
> 
> Here is a new attempt :) Fixing this made me want to do a little
> refactoring, so I've attached three patches:
> 
> 1) Rename Rename analyze_format_string::ArgTypeResult to ArgType and
> remove redundant constructors and unused functions.
> 
> 2) Remove ScanfArgTypeResult, and move the logic for expecting a
> "pointer to some ArgType" into ArgType itself, so it can be used for
> printf as well as for scanf.
> 
> 3) Use the functionality of ArgType to properly check the arguments for %n.
> 
> Please take a look.
> 
> Thanks,
> Hans
> <1_rename_argtyperesult.patch><2_remove_scanfargtype.patch><3_handle_n_length_modifiers.patch>