aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land.
LGTM! ================ Comment at: clang/test/SemaCXX/attr-format.cpp:76 + format("bare string"); + format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}} + format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, &do_format); ---------------- fcloutier wrote: > aaron.ballman wrote: > > This pointed out an interesting test case. What should the behavior be for: > > ``` > > format("%p", 0); > > ``` > > Because that sure feels like a more reasonable thing for someone to write > > expecting it to be treated as a null pointer constant. > I think that the current behavior is the right one: > > ``` > test.c:4:17: warning: format specifies type 'void *' but the argument has > type 'int' [-Wformat] > printf("%p\n", 0); > ~~ ^ > %d > ``` > > The warning goes away if you use `(void *)0`, as expected. > `__attribute__((format))` has no semantic meaning, so we can't (and > shouldn't) infer that 0 is a pointer based on the usage of %p. Ah, you know what, I've convinced myself I was wrong and you're right. C2x 7.22.6.1p9 gives the latest conversion rules here, and I think passing `0`, despite being the null pointer constant, is UB when the format specifier is `%p`. On targets where `int` and `void *` are the same width, this diagnostic feels rather pedantic. But on systems where those differ, it seems more important to issue the warning... so I think you're correct that we should leave this behavior alone. Thanks for thinking it through with me. :-) ================ Comment at: clang/test/SemaCXX/attr-format.cpp:77-78 + format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}} + format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, &do_format); + format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format); + format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}} ---------------- fcloutier wrote: > aaron.ballman wrote: > > This likely isn't specific to your changes, but the `%p` in these examples > > should be warning the user (a function or function pointer is not a pointer > > to void or a pointer to a character type, so that call is UB). > This is already a -Wformat-pedantic warning, which IMO is the right warning > group for it: > > ``` > test.c:4:17: warning: format specifies type 'void *' but the argument has > type 'int (*)()' [-Wformat-pedantic] > printf("%p\n", main); > ~~ ^~~~ > 1 warning generated. > ``` > > The relevant bit is clang/lib/AST/FormatString.cpp: > > ``` > case CPointerTy: > if (argTy->isVoidPointerType()) { > return Match; > } if (argTy->isPointerType() || argTy->isObjCObjectPointerType() || > argTy->isBlockPointerType() || argTy->isNullPtrType()) { > return NoMatchPedantic; > } else { > return NoMatch; > } > ``` Ah, good that we have it in a pedantic diagnostic. I agree, it is a pedantic one, I thought we were missing it entirely. 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