[cfe-commits] [PATCH] Teach printf warnings about small integer types.

[Ted Kremenek]

On Oct 2, 2010, at 12:49 AM, Justin Bogner wrote:

> Previously, the printf warnings would say your arguments type was
> `int` when it was really a `char` or a `short`. This fixes that and
> allows the hints to suggest `h` modifiers for small ints.
> 
> Typedef'd `char` types are treated as ints, since the most common case
> of uint8_t shouldn't normally be formatted as a character.
> ---
> lib/Analysis/PrintfFormatString.cpp |   16 +++++++++++++++-
> lib/Sema/SemaChecking.cpp           |    6 ++++--
> test/Sema/format-strings.c          |   11 ++++++++++-
> 3 files changed, 29 insertions(+), 4 deletions(-)
> 
> <informative-printf.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Hi Justin,

I was on vacation, so I'm sorry I took so long to respond.  Overall this looks good, but I have a few comments on the patch itself.  Comments inline:

-  if (QT->isAnyCharacterType()) {
+  // Let typedefs to char fall through to int, as %c is silly for uint8_t
+  if (!TypedefType::classof(QT.getTypePtr()) && QT->isAnyCharacterType()) {

Don't use classof() directly, use isa<> or dyn_cast<>, e.g.:

  if (isa<TypedefType>(QT) && QT->isAnyCharacterType())

For the following:

-      if (ICE->getType() == S.Context.IntTy)
-        if (ATR.matchesType(S.Context, ICE->getSubExpr()->getType()))
+      if (ICE->getType() == S.Context.IntTy) {
+        Ex = ICE->getSubExpr();
+        if (ATR.matchesType(S.Context, Ex->getType()))
           return true;
+      }

it looks like the code is mainly more verbose, except it now adds the side-effect that 'Ex' can change.  Please include a comment that this is intended, so that it doesn't look like a bug.

Otherwise, this looks great.  Please send a (slightly) modified patch with these requested changes and I'll go ahead and commit!