Page MenuHomePhabricator

[Sema] Don't warn on printf('%hd', [char]) (PR41467)
Needs RevisionPublic

Authored by Nathan-Huckleberry on Tue, Aug 13, 4:11 PM.

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 13, 4:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
  • Add comment in test
nickdesaulniers accepted this revision.Wed, Aug 14, 10:30 AM

LGTM thanks for the patch!

This revision is now accepted and ready to land.Wed, Aug 14, 10:30 AM
lebedev.ri retitled this revision from Fix warning on printf('%hd', [char]) to [Sema] Don't warn on printf('%hd', [char]) (PR41467).Wed, Aug 14, 10:54 AM
lebedev.ri added a reviewer: aaron.ballman.

There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does.

I could imagine "confusing user intent" being a valid reason why someone might want this warning, so we may want to default-off this diagnostic (because the code is safe) but still provide users with a way to enable it.

clang/test/Sema/format-strings-enum-fixed-type.cpp
82–83

This comment is now incorrect.

There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does.

Since printf is a variadic function, integral argument types are promoted to int. The warning code runs the matchesType check twice, once to check if the promoted type (int) is able to be printed with the format and once to check if the original type (char) is able to be printed with the format.

printf("%d", [char]) is caught by the first case
printf("%hhd", [char]) is caught by the second case.

printf("%hd", [char]) is a warning because an exception has not been made for that case.

There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does.

Since printf is a variadic function, integral argument types are promoted to int. The warning code runs the matchesType check twice, once to check if the promoted type (int) is able to be printed with the format and once to check if the original type (char) is able to be printed with the format.

printf("%d", [char]) is caught by the first case
printf("%hhd", [char]) is caught by the second case.

printf("%hd", [char]) is a warning because an exception has not been made for that case.

That explains what the implementation does, but does not attempt to answer the question *why* things are the way they are.

I read https://bugs.llvm.org/show_bug.cgi?id=41467#c4 as

  • any narrowing is always diagnosed
  • promotion to wider than int is diagnosed
  • passthrough is not diagnosed
  • promotion to something smaller than int is diagnosed (the current case)

I can interpret it as: we already know that

Since printf is a variadic function, integral argument types are promoted to int.

therefore why are you first implicitly promoting to int and then implicitly truncating?
Did you mean to print the original value? Did you mean to print int?

That doesn't sound too outlandish to me.

There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does.

Since printf is a variadic function, integral argument types are promoted to int. The warning code runs the matchesType check twice, once to check if the promoted type (int) is able to be printed with the format and once to check if the original type (char) is able to be printed with the format.

printf("%d", [char]) is caught by the first case
printf("%hhd", [char]) is caught by the second case.

printf("%hd", [char]) is a warning because an exception has not been made for that case.

This all makes sense as to how things work today, but I was more wondering why they worked that way in the first place. I'm especially interested to know whether this is diagnosed because it shows confusion of the user's intent, because that seems like a valuable behavior to retain (though perhaps it doesn't need to be default-on).

lebedev.ri requested changes to this revision.Thu, Aug 15, 12:09 PM

(just want to mark it as "unanswered questions")

This revision now requires changes to proceed.Thu, Aug 15, 12:09 PM

As far as I can tell this case was just overlooked. The original commit adding this change https://reviews.llvm.org/rG0208793e41018ac168412a3da8b2fba70aba9716 only allows chars to int and chars to chars. Another commit ignores typing of chars https://reviews.llvm.org/rG74e82bd190017d59d5d78b07dedca5b06b4547da. I did not see anything related to this particular case in previous commits.

aaron.ballman requested changes to this revision.Fri, Aug 16, 6:53 AM

As far as I can tell this case was just overlooked. The original commit adding this change https://reviews.llvm.org/rG0208793e41018ac168412a3da8b2fba70aba9716 only allows chars to int and chars to chars. Another commit ignores typing of chars https://reviews.llvm.org/rG74e82bd190017d59d5d78b07dedca5b06b4547da. I did not see anything related to this particular case in previous commits.

Hmm, it looks like, at least from this review, someone thought the behavior was for demonstrating user intent: http://llvm.org/viewvc/llvm-project?view=revision&revision=157961.

I've convinced myself that -Wformat should disable that diagnostic by default, but there is utility in keeping it exposed through a different format warning flag. It seems like -Wformat-pedantic should still diagnose this case.