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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to