sysemu/numa.h includes hw/boards.h just for the CPUArchId typedef, at
the cost of pulling in more than two dozen extra headers indirectly.
I could move the typedef from hw/boards.h to qemu/typedefs.h. But
it's used in just two headers: boards.h and numa.h.
I could move it to another header both its users include.
exec/cpu-common.h seems to be the least bad fit.
But I'm keeping this simple & stupid: declare the struct tag in
numa.h.
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
exec.c | 1 +
hw/core/machine-qmp-cmds.c | 1 +
hw/core/machine.c | 1 +
hw/mem/pc-dimm.c | 1 +
include/hw/boards.h | 2 +-
include/sysemu/numa.h | 9 +++++++--
6 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/exec.c b/exec.c
index 6d60fdfb1f..4d9e146c79 100644
--- a/exec.c
+++ b/exec.c
@@ -41,6 +41,7 @@
#if defined(CONFIG_USER_ONLY)
#include "qemu.h"
#else /* !CONFIG_USER_ONLY */
+#include "hw/boards.h"
#include "exec/memory.h"
#include "exec/ioport.h"
#include "sysemu/dma.h"
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index c83e8954e1..d8284671f0 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -9,6 +9,7 @@
#include "qemu/osdep.h"
#include "cpu.h"
+#include "hw/boards.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-machine.h"
#include "qapi/qmp/qerror.h"
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2c9c93983a..901f3fa905 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -23,6 +23,7 @@
#include "sysemu/numa.h"
#include "qemu/error-report.h"
#include "sysemu/qtest.h"
+#include "hw/boards.h"
#include "hw/pci/pci.h"
#include "hw/mem/nvdimm.h"
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 0c83dcd61e..fa90d4fc6c 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -19,6 +19,7 @@
*/
#include "qemu/osdep.h"
+#include "hw/boards.h"
#include "hw/mem/pc-dimm.h"
#include "hw/qdev-properties.h"
#include "migration/vmstate.h"
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 67e551636a..739d109fe1 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -86,7 +86,7 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
* @props - CPU object properties, initialized by board
* #vcpus_count - number of threads provided by @cpu object
*/
-typedef struct {
+typedef struct CPUArchId {
uint64_t arch_id;
int64_t vcpus_count;
CpuInstanceProperties props;
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 01a263eba2..4c4c1dee9b 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -4,7 +4,10 @@
#include "qemu/bitmap.h"
#include "sysemu/sysemu.h"
#include "sysemu/hostmem.h"
-#include "hw/boards.h"
+#include "qapi/qapi-types-machine.h"
+#include "exec/cpu-common.h"
+
+struct CPUArchId;
extern int nb_numa_nodes; /* Number of NUMA nodes */
extern bool have_numa_distance;
@@ -32,5 +35,7 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
int nb_nodes, ram_addr_t size);
void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
int nb_nodes, ram_addr_t size);
-void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
+void numa_cpu_pre_plug(const struct CPUArchId *slot, DeviceState *dev,
+ Error **errp);
+
#endif
--
2.21.0
On Fri, Jul 26, 2019 at 02:05:37PM +0200, Markus Armbruster wrote:
> sysemu/numa.h includes hw/boards.h just for the CPUArchId typedef, at
> the cost of pulling in more than two dozen extra headers indirectly.
>
> I could move the typedef from hw/boards.h to qemu/typedefs.h. But
> it's used in just two headers: boards.h and numa.h.
>
> I could move it to another header both its users include.
> exec/cpu-common.h seems to be the least bad fit.
>
> But I'm keeping this simple & stupid: declare the struct tag in
> numa.h.
>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> exec.c | 1 +
> hw/core/machine-qmp-cmds.c | 1 +
> hw/core/machine.c | 1 +
> hw/mem/pc-dimm.c | 1 +
> include/hw/boards.h | 2 +-
> include/sysemu/numa.h | 9 +++++++--
> 6 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 6d60fdfb1f..4d9e146c79 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -41,6 +41,7 @@
> #if defined(CONFIG_USER_ONLY)
> #include "qemu.h"
> #else /* !CONFIG_USER_ONLY */
> +#include "hw/boards.h"
> #include "exec/memory.h"
> #include "exec/ioport.h"
> #include "sysemu/dma.h"
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index c83e8954e1..d8284671f0 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -9,6 +9,7 @@
>
> #include "qemu/osdep.h"
> #include "cpu.h"
> +#include "hw/boards.h"
> #include "qapi/error.h"
> #include "qapi/qapi-commands-machine.h"
> #include "qapi/qmp/qerror.h"
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2c9c93983a..901f3fa905 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -23,6 +23,7 @@
> #include "sysemu/numa.h"
> #include "qemu/error-report.h"
> #include "sysemu/qtest.h"
> +#include "hw/boards.h"
> #include "hw/pci/pci.h"
> #include "hw/mem/nvdimm.h"
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 0c83dcd61e..fa90d4fc6c 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -19,6 +19,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "hw/boards.h"
> #include "hw/mem/pc-dimm.h"
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 67e551636a..739d109fe1 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -86,7 +86,7 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
> * @props - CPU object properties, initialized by board
> * #vcpus_count - number of threads provided by @cpu object
> */
> -typedef struct {
> +typedef struct CPUArchId {
> uint64_t arch_id;
> int64_t vcpus_count;
> CpuInstanceProperties props;
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 01a263eba2..4c4c1dee9b 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -4,7 +4,10 @@
> #include "qemu/bitmap.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/hostmem.h"
> -#include "hw/boards.h"
> +#include "qapi/qapi-types-machine.h"
This seems to be needed because of NodeInfo.
> +#include "exec/cpu-common.h"
This seems to be needed because of ram_addr_t.
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> +
> +struct CPUArchId;
>
I wonder if we should do the same for other struct types like
NodeInfo.
Why is it bad to require the inclusion of hw/boards.h just
because of CPUArchId, but acceptable to require the inclusion of
qapi-types-machine.h just to be able to write "NodeInfo *nodes"
instead of "struct NodeInfo *nodes" below?
> extern int nb_numa_nodes; /* Number of NUMA nodes */
> extern bool have_numa_distance;
> @@ -32,5 +35,7 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> int nb_nodes, ram_addr_t size);
> void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> int nb_nodes, ram_addr_t size);
> -void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
> +void numa_cpu_pre_plug(const struct CPUArchId *slot, DeviceState *dev,
> + Error **errp);
> +
> #endif
> --
> 2.21.0
>
--
Eduardo
Cc'ing a few more people who might be interested.
Eduardo Habkost <ehabkost@redhat.com> writes:
> On Fri, Jul 26, 2019 at 02:05:37PM +0200, Markus Armbruster wrote:
>> sysemu/numa.h includes hw/boards.h just for the CPUArchId typedef, at
>> the cost of pulling in more than two dozen extra headers indirectly.
>>
>> I could move the typedef from hw/boards.h to qemu/typedefs.h. But
>> it's used in just two headers: boards.h and numa.h.
>>
>> I could move it to another header both its users include.
>> exec/cpu-common.h seems to be the least bad fit.
>>
>> But I'm keeping this simple & stupid: declare the struct tag in
>> numa.h.
>>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> exec.c | 1 +
>> hw/core/machine-qmp-cmds.c | 1 +
>> hw/core/machine.c | 1 +
>> hw/mem/pc-dimm.c | 1 +
>> include/hw/boards.h | 2 +-
>> include/sysemu/numa.h | 9 +++++++--
>> 6 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 6d60fdfb1f..4d9e146c79 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -41,6 +41,7 @@
>> #if defined(CONFIG_USER_ONLY)
>> #include "qemu.h"
>> #else /* !CONFIG_USER_ONLY */
>> +#include "hw/boards.h"
>> #include "exec/memory.h"
>> #include "exec/ioport.h"
>> #include "sysemu/dma.h"
>> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
>> index c83e8954e1..d8284671f0 100644
>> --- a/hw/core/machine-qmp-cmds.c
>> +++ b/hw/core/machine-qmp-cmds.c
>> @@ -9,6 +9,7 @@
>>
>> #include "qemu/osdep.h"
>> #include "cpu.h"
>> +#include "hw/boards.h"
>> #include "qapi/error.h"
>> #include "qapi/qapi-commands-machine.h"
>> #include "qapi/qmp/qerror.h"
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 2c9c93983a..901f3fa905 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -23,6 +23,7 @@
>> #include "sysemu/numa.h"
>> #include "qemu/error-report.h"
>> #include "sysemu/qtest.h"
>> +#include "hw/boards.h"
>> #include "hw/pci/pci.h"
>> #include "hw/mem/nvdimm.h"
>>
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index 0c83dcd61e..fa90d4fc6c 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -19,6 +19,7 @@
>> */
>>
>> #include "qemu/osdep.h"
>> +#include "hw/boards.h"
>> #include "hw/mem/pc-dimm.h"
>> #include "hw/qdev-properties.h"
>> #include "migration/vmstate.h"
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 67e551636a..739d109fe1 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -86,7 +86,7 @@ void machine_class_allow_dynamic_sysbus_dev(MachineClass *mc, const char *type);
>> * @props - CPU object properties, initialized by board
>> * #vcpus_count - number of threads provided by @cpu object
>> */
>> -typedef struct {
>> +typedef struct CPUArchId {
>> uint64_t arch_id;
>> int64_t vcpus_count;
>> CpuInstanceProperties props;
>> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
>> index 01a263eba2..4c4c1dee9b 100644
>> --- a/include/sysemu/numa.h
>> +++ b/include/sysemu/numa.h
>> @@ -4,7 +4,10 @@
>> #include "qemu/bitmap.h"
>> #include "sysemu/sysemu.h"
>> #include "sysemu/hostmem.h"
>> -#include "hw/boards.h"
>> +#include "qapi/qapi-types-machine.h"
>
> This seems to be needed because of NodeInfo.
NumaOptions, actually.
>> +#include "exec/cpu-common.h"
>
> This seems to be needed because of ram_addr_t.
Correct.
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
>
>> +
>> +struct CPUArchId;
>>
>
> I wonder if we should do the same for other struct types like
> NodeInfo.
NumaOptions, I think.
> Why is it bad to require the inclusion of hw/boards.h just
> because of CPUArchId, but acceptable to require the inclusion of
> qapi-types-machine.h just to be able to write "NodeInfo *nodes"
> instead of "struct NodeInfo *nodes" below?
hw/boards.h is worse. Both headers pull in qapi/qapi-builtin-types.h
and qapi/qapi-types-common.h, but hw/boards.h pulls in ~60 more.
That doesn't mean including qapi/qapi-types-common.h is good.
For better or worse[*], our coding style mandates typedefs.
We generally declare a typedef name in exactly one place. The obvious
place is together with the type it denotes.
Non-local users of the type then need to include the header that
declares the typedef name.
This can lead to inclusion cycles, in particular for complete struct and
union types that need more types for their members.
We move typedefs to qemu/typedefs.h to break such cycles.
We also do it to include less, for less frequent recompilation. As this
series demonstrates, we're not very good at that. When to put a typedef
in qemu/typedefs.h for this purpose is not obvious. If we simply put
all of them there, we'd recompile the world every time we add a typedef,
which is pretty much exactly what we're trying to avoid.
Some of qemu/typedefs.h's typedefs are used in dozens or even hundreds
of other headers. Quite a few only in one. The latter likely should
not be there.
We occasionally give up and use types directly rather than their typedef
names, flouting the coding style. This patch does. Trades messing with
qemu/typedefs.h for having to write 'struct' a few times.
Should we give up more? Or not at all?
Can we have some guidelines on proper use of qemu/typedefs.h?
>> extern int nb_numa_nodes; /* Number of NUMA nodes */
>> extern bool have_numa_distance;
>> @@ -32,5 +35,7 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>> int nb_nodes, ram_addr_t size);
>> void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>> int nb_nodes, ram_addr_t size);
>> -void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
>> +void numa_cpu_pre_plug(const struct CPUArchId *slot, DeviceState *dev,
>> + Error **errp);
>> +
>> #endif
>> --
>> 2.21.0
>>
[*] Worse if you ask me.
On 7/30/19 6:01 AM, Markus Armbruster wrote: > Cc'ing a few more people who might be interested. > > Eduardo Habkost <ehabkost@redhat.com> writes: >> Why is it bad to require the inclusion of hw/boards.h just >> because of CPUArchId, but acceptable to require the inclusion of >> qapi-types-machine.h just to be able to write "NodeInfo *nodes" >> instead of "struct NodeInfo *nodes" below? > > hw/boards.h is worse. Both headers pull in qapi/qapi-builtin-types.h > and qapi/qapi-types-common.h, but hw/boards.h pulls in ~60 more. > > That doesn't mean including qapi/qapi-types-common.h is good. > > For better or worse[*], our coding style mandates typedefs. I could live with a switch to kernel style of always writing the 'struct' at every use, instead of using typedefs; but it would have to be a flag-day switchover, preferably aided by Coccinelle. But I'm not holding my breath for it happening. > > We generally declare a typedef name in exactly one place. The obvious > place is together with the type it denotes. > > Non-local users of the type then need to include the header that > declares the typedef name. > > This can lead to inclusion cycles, in particular for complete struct and > union types that need more types for their members. > > We move typedefs to qemu/typedefs.h to break such cycles. > > We also do it to include less, for less frequent recompilation. As this > series demonstrates, we're not very good at that. When to put a typedef > in qemu/typedefs.h for this purpose is not obvious. If we simply put > all of them there, we'd recompile the world every time we add a typedef, > which is pretty much exactly what we're trying to avoid. > > Some of qemu/typedefs.h's typedefs are used in dozens or even hundreds > of other headers. Quite a few only in one. The latter likely should > not be there. > > We occasionally give up and use types directly rather than their typedef > names, flouting the coding style. This patch does. Trades messing with > qemu/typedefs.h for having to write 'struct' a few times. > > Should we give up more? Or not at all? > > Can we have some guidelines on proper use of qemu/typedefs.h? How hard would it be to compute which typedefs already in qemu/typedefs.h are necessary to avoid cyclic inclusion? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 30/07/19 15:15, Eric Blake wrote: >> We occasionally give up and use types directly rather than their typedef >> names, flouting the coding style. This patch does. Trades messing with >> qemu/typedefs.h for having to write 'struct' a few times. I think Markus made the right call here. Using "struct Foo;" in headers is a null price to pay if all you need is declaring a pointer-typed field or parameter. Of course this doesn't apply if you have to embed a struct directly, but then qemu/typedefs.h wouldn't help either. In general unless you're adding a new subsystem, qemu/typedefs.h should only decrease in size, never increase. (And there are certainly many cases where typedefs.h are not needed, but cleaning that up is understandably not high on the todo list). Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 30/07/19 15:15, Eric Blake wrote: >>> We occasionally give up and use types directly rather than their typedef >>> names, flouting the coding style. This patch does. Trades messing with >>> qemu/typedefs.h for having to write 'struct' a few times. > > I think Markus made the right call here. Using "struct Foo;" in headers > is a null price to pay if all you need is declaring a pointer-typed > field or parameter. Eduardo posted a patch to HACKING to clarify this non-usage of typedef is okay. Should we continue to mandate typedef names elsewhere? It adds cognitive load: you have to decide where to put the typedef, and when not to use it. > Of course this doesn't apply if you have to embed a > struct directly, but then qemu/typedefs.h wouldn't help either. Yes, and if this leads to an inclusion cycle, I strongly suspect "fat" headers: since you can't embed something in itself, the cycle must involve different things, all bunched together in the same header. > In general unless you're adding a new subsystem, qemu/typedefs.h should > only decrease in size, never increase. This series grows it some. I'll try to avoid that for v2. > (And there are certainly many > cases where typedefs.h are not needed, but cleaning that up is > understandably not high on the todo list). On the other hand, low-hanging fruit.
On 31/07/19 08:37, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 30/07/19 15:15, Eric Blake wrote: >>>> We occasionally give up and use types directly rather than their typedef >>>> names, flouting the coding style. This patch does. Trades messing with >>>> qemu/typedefs.h for having to write 'struct' a few times. >> >> I think Markus made the right call here. Using "struct Foo;" in headers >> is a null price to pay if all you need is declaring a pointer-typed >> field or parameter. > > Eduardo posted a patch to HACKING to clarify this non-usage of typedef > is okay. > > Should we continue to mandate typedef names elsewhere? It adds > cognitive load: you have to decide where to put the typedef, and when > not to use it. A lot of libraries we use (especially GLib) use typedefs, so I'd rather keep them. Also, a mass replacement would be tens of thousands of changed lines and not really practical. >> Of course this doesn't apply if you have to embed a >> struct directly, but then qemu/typedefs.h wouldn't help either. > > Yes, and if this leads to an inclusion cycle, I strongly suspect "fat" > headers: since you can't embed something in itself, the cycle must > involve different things, all bunched together in the same header. > >> In general unless you're adding a new subsystem, qemu/typedefs.h should >> only decrease in size, never increase. > > This series grows it some. I'll try to avoid that for v2. Let me review it first. :) Maybe some of them are good to keep. Paolo
On 31/07/2019 08.37, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 30/07/19 15:15, Eric Blake wrote: >>>> We occasionally give up and use types directly rather than their typedef >>>> names, flouting the coding style. This patch does. Trades messing with >>>> qemu/typedefs.h for having to write 'struct' a few times. >> >> I think Markus made the right call here. Using "struct Foo;" in headers >> is a null price to pay if all you need is declaring a pointer-typed >> field or parameter. > > Eduardo posted a patch to HACKING to clarify this non-usage of typedef > is okay. > > Should we continue to mandate typedef names elsewhere? It adds > cognitive load: you have to decide where to put the typedef, and when > not to use it. IMHO we should get rid of mandating typedefs. They are causing too much trouble - e.g. do you also remember the issues with duplicated typedefs in certain compiler versions in the past? (these should be hopefully gone now, but still...) And many QEMU developers are also working on the Linux kernel, which rather forbids typedefs. Having to switch your mind back and forth whether to use typedefs or not is really annoying. So if you ask me, stop mandating it! It's ok as optional feature in QEMU for types that are used all over the place, but we really should not enforce it for each and every struct anymore. Thomas
On Wed, 31 Jul 2019 at 09:40, Thomas Huth <thuth@redhat.com> wrote: > IMHO we should get rid of mandating typedefs. They are causing too much > trouble - e.g. do you also remember the issues with duplicated typedefs > in certain compiler versions in the past? (these should be hopefully > gone now, but still...) > > And many QEMU developers are also working on the Linux kernel, which > rather forbids typedefs. Having to switch your mind back and forth > whether to use typedefs or not is really annoying. I would rather keep typedefs -- it's one of the style issues we're reasonably consistent with. QEMU isn't the kernel, and its style is not the same on many points. If we switch to "use 'struct Foo'" we'll have a codebase which becomes rapidly very inconsistent about whether we use 'struct' or not. thanks -- PMM
On Wed, Jul 31, 2019 at 11:45:41AM +0100, Peter Maydell wrote: > On Wed, 31 Jul 2019 at 09:40, Thomas Huth <thuth@redhat.com> wrote: > > IMHO we should get rid of mandating typedefs. They are causing too much > > trouble - e.g. do you also remember the issues with duplicated typedefs > > in certain compiler versions in the past? (these should be hopefully > > gone now, but still...) > > > > And many QEMU developers are also working on the Linux kernel, which > > rather forbids typedefs. Having to switch your mind back and forth > > whether to use typedefs or not is really annoying. > > I would rather keep typedefs -- it's one of the style issues we're > reasonably consistent with. QEMU isn't the kernel, and its style > is not the same on many points. If we switch to "use 'struct Foo'" > we'll have a codebase which becomes rapidly very inconsistent > about whether we use 'struct' or not. I tend to agree - while people may work on kernel code, plenty do not work on kernel code & QEMU is not following kernel code pratices more generally. I think it is more compelling to align with glib given that it is a core part of QEMU codebase. I'd much rather QEMU more closely align with glib and increasingly drop stuff that QEMU has reinvented in favour of using GLib features. For example I could see GObject as a base for QOM in future, and typedefs are a normal practice in this case. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Sometimes we use the 'struct' keyword to help us reduce
dependencies between header files. Document that practice.
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
I wonder if this is too terse? Should we give examples?
---
HACKING | 2 ++
1 file changed, 2 insertions(+)
diff --git a/HACKING b/HACKING
index 0fc3e0fc04..112685bdaf 100644
--- a/HACKING
+++ b/HACKING
@@ -101,6 +101,8 @@ it points to, or it is aliased to another pointer that is.
2.3. Typedefs
Typedefs are used to eliminate the redundant 'struct' keyword.
+However, the 'struct' keyword may be sometimes used in header
+files to avoid unnecessary dependencies between headers.
2.4. Reserved namespaces in C and POSIX
Underscore capital, double underscore, and underscore 't' suffixes should be
--
2.21.0
On 7/30/19 4:07 PM, Eduardo Habkost wrote: > Sometimes we use the 'struct' keyword to help us reduce > dependencies between header files. Document that practice. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > I wonder if this is too terse? Should we give examples? An example might be nice, but I like the wording - it makes it obvious that the header uses 'struct' but the .c should use the typedef. > --- > HACKING | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/HACKING b/HACKING > index 0fc3e0fc04..112685bdaf 100644 > --- a/HACKING > +++ b/HACKING > @@ -101,6 +101,8 @@ it points to, or it is aliased to another pointer that is. > > 2.3. Typedefs > Typedefs are used to eliminate the redundant 'struct' keyword. > +However, the 'struct' keyword may be sometimes used in header > +files to avoid unnecessary dependencies between headers. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 30/07/2019 23.07, Eduardo Habkost wrote: > Sometimes we use the 'struct' keyword to help us reduce > dependencies between header files. Document that practice. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > I wonder if this is too terse? Should we give examples? > --- > HACKING | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/HACKING b/HACKING > index 0fc3e0fc04..112685bdaf 100644 > --- a/HACKING > +++ b/HACKING > @@ -101,6 +101,8 @@ it points to, or it is aliased to another pointer that is. > > 2.3. Typedefs > Typedefs are used to eliminate the redundant 'struct' keyword. > +However, the 'struct' keyword may be sometimes used in header > +files to avoid unnecessary dependencies between headers. See also the discussion earlier this year: https://www.mail-archive.com/qemu-devel@nongnu.org/msg586180.html ... and we should merge HACKING and CODING_STYLE finally (that was on my private TODO list, but I never found the time to do it). Thomas
On Wed, Jul 31, 2019 at 10:35:31AM +0200, Thomas Huth wrote:
> On 30/07/2019 23.07, Eduardo Habkost wrote:
> > Sometimes we use the 'struct' keyword to help us reduce
> > dependencies between header files. Document that practice.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > I wonder if this is too terse? Should we give examples?
> > ---
> > HACKING | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/HACKING b/HACKING
> > index 0fc3e0fc04..112685bdaf 100644
> > --- a/HACKING
> > +++ b/HACKING
> > @@ -101,6 +101,8 @@ it points to, or it is aliased to another pointer that is.
> >
> > 2.3. Typedefs
> > Typedefs are used to eliminate the redundant 'struct' keyword.
> > +However, the 'struct' keyword may be sometimes used in header
> > +files to avoid unnecessary dependencies between headers.
>
> See also the discussion earlier this year:
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg586180.html
Nice, it adds even more information than this patch.
For reference, this is the patch at the URL above:
-Typedefs are used to eliminate the redundant 'struct' keyword.
+Typedefs can be used to eliminate the redundant 'struct' keyword. This is
+especially helpful for common types that are used all over the place. Since
+certain C compilers choke on duplicated typedefs, you should avoid them and
+declare a typedef only in one header file. For common types, you can use
+"include/qemu/typedefs.h" for example. Note that it is also perfectly fine to
+use forward struct definitions without typedefs for references in headers
+to avoid the problem with duplicated typedefs.
I don't agree with the first two sentences, and I agree with what Paolo said
here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg586214.html
("I agree 100% with the wording after 'Since'. However, I think the first
part should be made stronger, not weaker.")
Paolo sent the following proposal:
| Typedefs are use to eliminate the redundant 'struct' keyword, since type
| names have a different style than other identifiers ("CamelCase" versus
| "snake_case"). Each struct should have a CamelCase name and a
| corresponding typedef.
|
| Since certain C compilers choke on duplicated typedefs, you should avoid
| them and declare a typedef only in one header file. For common types,
| you can use "include/qemu/typedefs.h" for example. However, as a metter
| of convenience it is also perfectly fine to use forward struct
| definitions instead of typedefs in headers and function prototypes; this
| avoids problems with duplicated typedefs and reduces the need to include
| headers from other headers.
It seems perfect to me.
Paolo, do I have your signed-off-by to send that in a patch?
>
> ... and we should merge HACKING and CODING_STYLE finally (that was on my
> private TODO list, but I never found the time to do it).
Agreed, but I prefer to fix one problem at a time.
--
Eduardo
On 01/08/19 20:50, Eduardo Habkost wrote:
> On Wed, Jul 31, 2019 at 10:35:31AM +0200, Thomas Huth wrote:
>> On 30/07/2019 23.07, Eduardo Habkost wrote:
>>> Sometimes we use the 'struct' keyword to help us reduce
>>> dependencies between header files. Document that practice.
>>>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>> I wonder if this is too terse? Should we give examples?
>>> ---
>>> HACKING | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/HACKING b/HACKING
>>> index 0fc3e0fc04..112685bdaf 100644
>>> --- a/HACKING
>>> +++ b/HACKING
>>> @@ -101,6 +101,8 @@ it points to, or it is aliased to another pointer that is.
>>>
>>> 2.3. Typedefs
>>> Typedefs are used to eliminate the redundant 'struct' keyword.
>>> +However, the 'struct' keyword may be sometimes used in header
>>> +files to avoid unnecessary dependencies between headers.
>>
>> See also the discussion earlier this year:
>>
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg586180.html
>
> Nice, it adds even more information than this patch.
>
> For reference, this is the patch at the URL above:
>
> -Typedefs are used to eliminate the redundant 'struct' keyword.
> +Typedefs can be used to eliminate the redundant 'struct' keyword. This is
> +especially helpful for common types that are used all over the place. Since
> +certain C compilers choke on duplicated typedefs, you should avoid them and
> +declare a typedef only in one header file. For common types, you can use
> +"include/qemu/typedefs.h" for example. Note that it is also perfectly fine to
> +use forward struct definitions without typedefs for references in headers
> +to avoid the problem with duplicated typedefs.
>
> I don't agree with the first two sentences, and I agree with what Paolo said
> here:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg586214.html
>
> ("I agree 100% with the wording after 'Since'. However, I think the first
> part should be made stronger, not weaker.")
>
> Paolo sent the following proposal:
>
> | Typedefs are use to eliminate the redundant 'struct' keyword, since type
> | names have a different style than other identifiers ("CamelCase" versus
> | "snake_case"). Each struct should have a CamelCase name and a
> | corresponding typedef.
> |
> | Since certain C compilers choke on duplicated typedefs, you should avoid
> | them and declare a typedef only in one header file. For common types,
> | you can use "include/qemu/typedefs.h" for example. However, as a metter
> | of convenience it is also perfectly fine to use forward struct
> | definitions instead of typedefs in headers and function prototypes; this
> | avoids problems with duplicated typedefs and reduces the need to include
> | headers from other headers.
>
> It seems perfect to me.
>
> Paolo, do I have your signed-off-by to send that in a patch?
Sure.
Paolo
On 01/08/2019 21.23, Paolo Bonzini wrote:
> On 01/08/19 20:50, Eduardo Habkost wrote:
>> On Wed, Jul 31, 2019 at 10:35:31AM +0200, Thomas Huth wrote:
>>> On 30/07/2019 23.07, Eduardo Habkost wrote:
>>>> Sometimes we use the 'struct' keyword to help us reduce
>>>> dependencies between header files. Document that practice.
>>>>
>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>>> ---
>>>> I wonder if this is too terse? Should we give examples?
>>>> ---
>>>> HACKING | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/HACKING b/HACKING
>>>> index 0fc3e0fc04..112685bdaf 100644
>>>> --- a/HACKING
>>>> +++ b/HACKING
>>>> @@ -101,6 +101,8 @@ it points to, or it is aliased to another pointer that is.
>>>>
>>>> 2.3. Typedefs
>>>> Typedefs are used to eliminate the redundant 'struct' keyword.
>>>> +However, the 'struct' keyword may be sometimes used in header
>>>> +files to avoid unnecessary dependencies between headers.
>>>
>>> See also the discussion earlier this year:
>>>
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg586180.html
>>
>> Nice, it adds even more information than this patch.
>>
>> For reference, this is the patch at the URL above:
>>
>> -Typedefs are used to eliminate the redundant 'struct' keyword.
>> +Typedefs can be used to eliminate the redundant 'struct' keyword. This is
>> +especially helpful for common types that are used all over the place. Since
>> +certain C compilers choke on duplicated typedefs, you should avoid them and
>> +declare a typedef only in one header file. For common types, you can use
>> +"include/qemu/typedefs.h" for example. Note that it is also perfectly fine to
>> +use forward struct definitions without typedefs for references in headers
>> +to avoid the problem with duplicated typedefs.
>>
>> I don't agree with the first two sentences, and I agree with what Paolo said
>> here:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg586214.html
>>
>> ("I agree 100% with the wording after 'Since'. However, I think the first
>> part should be made stronger, not weaker.")
>>
>> Paolo sent the following proposal:
>>
>> | Typedefs are use to eliminate the redundant 'struct' keyword, since type
s/use/used/
>> | names have a different style than other identifiers ("CamelCase" versus
>> | "snake_case"). Each struct should have a CamelCase name and a
>> | corresponding typedef.
>> |
>> | Since certain C compilers choke on duplicated typedefs, you should avoid
>> | them and declare a typedef only in one header file. For common types,
>> | you can use "include/qemu/typedefs.h" for example. However, as a metter
s/metter/matter/
>> | of convenience it is also perfectly fine to use forward struct
>> | definitions instead of typedefs in headers and function prototypes; this
>> | avoids problems with duplicated typedefs and reduces the need to include
>> | headers from other headers.
With the typos fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>
On Tue, Jul 30, 2019 at 11:07 PM Eduardo Habkost <ehabkost@redhat.com> wrote: > Sometimes we use the 'struct' keyword to help us reduce > dependencies between header files. Document that practice. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > I wonder if this is too terse? Should we give examples? > --- > I am inclined to have the judgement very similar to Eric's - it seems to me the change is fine as is, so: Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com> > HACKING | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/HACKING b/HACKING > index 0fc3e0fc04..112685bdaf 100644 > --- a/HACKING > +++ b/HACKING > @@ -101,6 +101,8 @@ it points to, or it is aliased to another pointer that > is. > > 2.3. Typedefs > Typedefs are used to eliminate the redundant 'struct' keyword. > +However, the 'struct' keyword may be sometimes used in header > +files to avoid unnecessary dependencies between headers. > > 2.4. Reserved namespaces in C and POSIX > Underscore capital, double underscore, and underscore 't' suffixes should > be > -- > 2.21.0 > >
On Tue, Jul 30, 2019 at 08:15:12AM -0500, Eric Blake wrote: > How hard would it be to compute which typedefs already in > qemu/typedefs.h are necessary to avoid cyclic inclusion? I don't think it's just about cyclic inclusion. It's also about avoiding dependencies between headers just because of a single typedef, like numa.h including hw/boards.h just because of the CPUArchId typedef. -- Eduardo
© 2016 - 2026 Red Hat, Inc.