Quuxplusone added a comment.
> Agreed. This should be checking the instantiations, so by that point, the
> variadic template is really more like a fixed parameter list anyway.
FWIW, in my own mental model, there's a big semantic difference between
(varargs functions, variadic templates) on the one hand and (non-template
functions) on the other. In my experience, there's //nothing// you can do with
a varargs ellipsis except forward it along as a `va_list`; and it's
//uncommon// to do anything with a variadic parameter pack except forward it
along via `std::forward`; but messing with fixed arguments is quite common.
Technically, you can mess with a parameter pack too:
void apple(const char *fmt, int x, int y) __attribute__((format(printf, 1,
2))) {
printf(fmt, double(x), double(y));
}
void banana(const char *fmt, auto... args) __attribute__((format(printf, 1,
2))) {
printf(fmt, double(args)...);
}
int main() {
apple("%g %g", 17, 42); // well-defined; shall we warn anyway? (My gut
feeling is that this is relatively common)
banana("%g %g", 17, 42); // well-defined; shall we warn anyway? (My gut
feeling is that this is extremely rare)
}
This morning I am leaning in favor of this PR as written. If a programmer wants
`apple`/`banana` to be callable like that, without any diagnostics, then their
appropriate course of action is simply not to apply the
`__attribute__((format(printf, 1, 2)))` annotation.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4051
+def warn_gcc_requires_variadic_function : Warning<
+ "GCC requires functions with %0 attribute to be variadic">,
+ InGroup<GccCompat>;
----------------
I'd say `with the %0 attribute` (add "the")
================
Comment at: clang/lib/AST/FormatString.cpp:325-329
+ // When using the format attribute with variadic templates in C++, you can
+ // receive an array that will necessarily decay to a pointer when passed to
+ // the final format consumer. Apply decay before type comparison.
+ if (C.getAsArrayType(argTy))
+ argTy = C.getArrayDecayedType(argTy);
----------------
Also, function references will decay to function pointers. I have no idea if
you need to do anything special here to get the "right behavior" for function
references. But please add a (compile-only?) test case for the
function-pointer codepath, just to prove it doesn't crash or anything.
================
Comment at: clang/test/Sema/attr-format.cpp:5-7
+template<typename... Args>
+void format(const char *fmt, Args &&... args) // expected-warning{{GCC
requires functions with 'format' attribute to be variadic}}
+ __attribute__((format(printf, 1, 2)));
----------------
Can we also do an example of a //member function// variadic template?
```
struct S {
template<class... Args>
void format(const char *fmt, Args&&... args)
__attribute__((format(printf, 2, 3)));
};
```
Also, I believe that this entire file should be removed from `Sema/` and
combined into `SemaCXX/attr-format.cpp`.
I also notice that we have literally zero test coverage for cases where the
format string is not the first argument to the function; but that
can-and-should be addressed in a separate PR.
================
Comment at: clang/test/Sema/attr-format.cpp:14
+ format("bare string");
+ 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}}
----------------
Basically, add `, do_format` here.
(Aside: I'm surprised that Clang quietly lets you print a function pointer with
`%p`. It'll work on sane architectures, but //in general// C and C++ don't
require that function pointers even be the same size as `void*` — technically
this should be UB or at least impl-defined behavior.)
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
