Re: [Qemu-devel] [PATCH 1/2] fprintf-fn.h uses GCC_FMT, but doesn't include the definition of it

2017-09-25 Thread Daniel Loffgren 
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  wrote:
> 
> On 09/24/2017 08:02 PM, Daniel Loffgren wrote:
>> 
>> Signed-off-by: Daniel Loffgren 
> 
> 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
>

Re: [Qemu-devel] [PATCH 1/2] fprintf-fn.h uses GCC_FMT, but doesn't include the definition of it

2017-09-25 Thread Eric Blake 
On 09/24/2017 08:02 PM, Daniel Loffgren wrote:
> 
> Signed-off-by: Daniel Loffgren 

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



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 1/2] fprintf-fn.h uses GCC_FMT, but doesn't include the definition of it

2017-09-25 Thread Daniel Loffgren 

Signed-off-by: Daniel Loffgren 
---
 include/qemu/fprintf-fn.h | 2 ++
 1 file changed, 2 insertions(+)

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);
 
--