[PATCH] D112579: Allow non-variadic functions to be attributed with `__attribute__((format))`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for this! I have some concerns that I'd like to talk out to hopefully make myself feel more comfortable with this.

My understanding of the utility from -Wformat warnings comes from the fact that the function receiving the format string has no way to validate that the variadic arguments passed have the correct types when compared to the format string. However, in both of the new cases you propose to support, that type information is present in the function signature (you may have to go through a bunch of template instantiations in the variadic template cases, but you get there eventually). Having the types in the signatures changes something I think might be pretty fundamental to the way the format string checker works -- it tries to figure out types *after default argument promotions* because those promotions are a lossy operation. However, when the types are specified in the signature, those default argument promotions no longer happen. The type passed for a `%f` may actually be a `float` rather than promoted to a `double`, the type for a `char` may actually be `char` rather than `int`, etc. I'm not convinced this is as simple as "now we allow non-vararg functions" for the implementation. I think we need some more extensive testing to prove that the diagnostics actually make sense or not.

In terms of the specific cases allowed, I think I am happier about variadic templates than I am about fixed-signature functions. For the variadic template, I think supporting -Wformat would mean users don't have to reimplement similar logic to what that check already handles even though they can do it themselves. But for a fixed-signature function, I'm not certain I see what the value add is -- the only valid format strings such a function could accept would have to be fixed to the function signature anyway. Can you explain what use cases you have for that situation?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112579/new/

https://reviews.llvm.org/D112579