| Message ID | 40DEFE92-410B-4FED-B0A5-56C186627134@loffgren.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] fprintf-fn.h uses GCC_FMT, but doesn't include the definition of it | expand |
On 09/24/2017 08:02 PM, Daniel Loffgren wrote: > > Signed-off-by: Daniel Loffgren <daniel@loffgren.org> Hmm - you've identified a file with no listed maintainer. But qemu-trivial does seem like the right place to include it. Generally, we like patches to call out the topic that it is touching; also, the subject line should focus on the what, while the body focuses on the why. So a better commit message might be: maint: Make fprintf-fn.h self-contained Include the necessary headers so that GCC_FMT_ATTR is defined regardless of what client files use fprintf-fn.h. However, after saying that, I think your patch is not needed. Per HACKING, _all_ .c files must include osdep.h first, and osdep.h already includes compiler.h, therefore, any .c file that uses fprintf-fn.h already has GCC_FMT_ATTR in scope by the time it gets to the fprintf_function typedef. If you ran into a situation where you had a compile failure, please post more details of what failed for you, in case the problem was you forgetting to use osdep.h.
I am working on getting ppc-darwin-user working again, and this was one of the many things preventing it from compiling. Your explanation sounds correct. I believe it was the lack of osdep.h in the right .c files, since I added osdep.h to the tops of all of the necessary files well after this change (I was fixing things in order of compiler failures). I dropped this commit from my branch and it didn’t break. So, feel free to disregard this. > On Sep 25, 2017, at 9:18 AM, Eric Blake <eblake@redhat.com> wrote: > > On 09/24/2017 08:02 PM, Daniel Loffgren wrote: >> >> Signed-off-by: Daniel Loffgren <daniel@loffgren.org> > > Hmm - you've identified a file with no listed maintainer. But > qemu-trivial does seem like the right place to include it. > > Generally, we like patches to call out the topic that it is touching; > also, the subject line should focus on the what, while the body focuses > on the why. So a better commit message might be: > > maint: Make fprintf-fn.h self-contained > > Include the necessary headers so that GCC_FMT_ATTR is defined regardless > of what client files use fprintf-fn.h. > > > However, after saying that, I think your patch is not needed. Per > HACKING, _all_ .c files must include osdep.h first, and osdep.h already > includes compiler.h, therefore, any .c file that uses fprintf-fn.h > already has GCC_FMT_ATTR in scope by the time it gets to the > fprintf_function typedef. If you ran into a situation where you had a > compile failure, please post more details of what failed for you, in > case the problem was you forgetting to use osdep.h. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
diff --git a/include/qemu/fprintf-fn.h b/include/qemu/fprintf-fn.h index 9068a960b3..80361d87bf 100644 --- a/include/qemu/fprintf-fn.h +++ b/include/qemu/fprintf-fn.h @@ -8,6 +8,8 @@ #ifndef QEMU_FPRINTF_FN_H #define QEMU_FPRINTF_FN_H +#include "compiler.h" + typedef int (*fprintf_function)(FILE *f, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
Signed-off-by: Daniel Loffgren <daniel@loffgren.org> --- include/qemu/fprintf-fn.h | 2 ++ 1 file changed, 2 insertions(+) --