| Message ID | 20190111140857.4211-4-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | typedefs: Remove scarcely used declarations | expand |
On 2019-01-11 15:08, Philippe Mathieu-Daudé wrote: > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > There are only three files requiring this typedef, let them s/files/header files/ Reviewed-by: Thomas Huth <thuth@redhat.com> > include "hw/ssi/ssi.h" directly to simplify "qemu/typedefs.h". > > To clean "qemu/typedefs.h", move the forward declaration > to "hw/ssi/ssi.h". > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/arm/strongarm.h | 1 + > include/hw/arm/pxa.h | 1 + > include/hw/ssi/pl022.h | 1 + > include/hw/ssi/ssi.h | 1 + > include/qemu/typedefs.h | 1 - > 5 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/strongarm.h b/hw/arm/strongarm.h > index e98840b461..ae51a1ae34 100644 > --- a/hw/arm/strongarm.h > +++ b/hw/arm/strongarm.h > @@ -3,6 +3,7 @@ > > #include "exec/memory.h" > #include "target/arm/cpu-qom.h" > +#include "hw/ssi/ssi.h" > > #define SA_CS0 0x00000000 > #define SA_CS1 0x08000000 > diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h > index f6dfb5c0cf..f184349f02 100644 > --- a/include/hw/arm/pxa.h > +++ b/include/hw/arm/pxa.h > @@ -13,6 +13,7 @@ > #include "exec/memory.h" > #include "target/arm/cpu-qom.h" > #include "hw/pcmcia.h" > +#include "hw/ssi/ssi.h" > > /* Interrupt numbers */ > # define PXA2XX_PIC_SSP3 0 > diff --git a/include/hw/ssi/pl022.h b/include/hw/ssi/pl022.h > index a080519366..1cf16f1539 100644 > --- a/include/hw/ssi/pl022.h > +++ b/include/hw/ssi/pl022.h > @@ -22,6 +22,7 @@ > #define HW_SSI_PL022_H > > #include "hw/sysbus.h" > +#include "hw/ssi/ssi.h" > > #define TYPE_PL022 "pl022" > #define PL022(obj) OBJECT_CHECK(PL022State, (obj), TYPE_PL022) > diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h > index 6a0c3c3cdb..bdbf3c51f5 100644 > --- a/include/hw/ssi/ssi.h > +++ b/include/hw/ssi/ssi.h > @@ -13,6 +13,7 @@ > > #include "hw/qdev.h" > > +typedef struct SSIBus SSIBus; > typedef struct SSISlave SSISlave; > typedef struct SSISlaveClass SSISlaveClass; > typedef enum SSICSMode SSICSMode; > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h > index 3bd9215d55..c026229573 100644 > --- a/include/qemu/typedefs.h > +++ b/include/qemu/typedefs.h > @@ -108,7 +108,6 @@ typedef struct Range Range; > typedef struct SerialState SerialState; > typedef struct SHPCDevice SHPCDevice; > typedef struct SMBusDevice SMBusDevice; > -typedef struct SSIBus SSIBus; > typedef struct uWireSlave uWireSlave; > typedef struct VirtIODevice VirtIODevice; > typedef struct Visitor Visitor; >
Philippe Mathieu-Daudé <philmd@redhat.com> writes: > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > There are only three files requiring this typedef, let them > include "hw/ssi/ssi.h" directly to simplify "qemu/typedefs.h". > > To clean "qemu/typedefs.h", move the forward declaration > to "hw/ssi/ssi.h". > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/arm/strongarm.h | 1 + > include/hw/arm/pxa.h | 1 + > include/hw/ssi/pl022.h | 1 + > include/hw/ssi/ssi.h | 1 + > include/qemu/typedefs.h | 1 - > 5 files changed, 4 insertions(+), 1 deletion(-) When typedefs.h changes, we recompile the world, but it pretty much only ever changes when new typedefs are added. Thus, *keeping* a typedef there is therefore pretty cheap. Nevertheless, we shouldn't keep typedefs there without a real reason. Being able to move one away without having to add any new #include directives is a strong sign for "no real reason". I like patches doing that. What I don't like is adding #include directives just so you can move typedefs out of typedefs.h: it slows down the build. Granted, the four added by this patch are a drop in the bucket. The point I'm trying to make is typedefs.h's purpose: it's for avoiding #include directives. Circular ones in particular, but others, too.
On 2019-01-15 13:28, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@redhat.com> writes: > >> From: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> There are only three files requiring this typedef, let them >> include "hw/ssi/ssi.h" directly to simplify "qemu/typedefs.h". >> >> To clean "qemu/typedefs.h", move the forward declaration >> to "hw/ssi/ssi.h". >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/arm/strongarm.h | 1 + >> include/hw/arm/pxa.h | 1 + >> include/hw/ssi/pl022.h | 1 + >> include/hw/ssi/ssi.h | 1 + >> include/qemu/typedefs.h | 1 - >> 5 files changed, 4 insertions(+), 1 deletion(-) > > When typedefs.h changes, we recompile the world, but it pretty much only > ever changes when new typedefs are added. Thus, *keeping* a typedef > there is therefore pretty cheap. > > Nevertheless, we shouldn't keep typedefs there without a real reason. > Being able to move one away without having to add any new #include > directives is a strong sign for "no real reason". I like patches doing > that. > > What I don't like is adding #include directives just so you can move > typedefs out of typedefs.h: it slows down the build. Granted, the four > added by this patch are a drop in the bucket. The point I'm trying to > make is typedefs.h's purpose: it's for avoiding #include directives. > Circular ones in particular, but others, too. Yes, agreed, removing things from typedefs.h just to add lots of #includes in other files is not really the best idea. I also dropped this patch in v2 of my current PULL request because of this reason. Phil, I suggest to simply drop this patch. What we maybe could do is to split up typedefs.h per subsystem, so that we additionally have a hw/arm/typedefs.h, hw/ppc/typedefs.h etc. in the end, then the target-specific typedefs would not clutter the common qemu/typedefs.h file anymore. Thomas
On 15/01/19 13:28, Markus Armbruster wrote: >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/arm/strongarm.h | 1 + >> include/hw/arm/pxa.h | 1 + >> include/hw/ssi/pl022.h | 1 + >> include/hw/ssi/ssi.h | 1 + >> include/qemu/typedefs.h | 1 - >> 5 files changed, 4 insertions(+), 1 deletion(-) > When typedefs.h changes, we recompile the world, but it pretty much only > ever changes when new typedefs are added. Thus, *keeping* a typedef > there is therefore pretty cheap. > > Nevertheless, we shouldn't keep typedefs there without a real reason. > Being able to move one away without having to add any new #include > directives is a strong sign for "no real reason". I like patches doing > that. > > What I don't like is adding #include directives just so you can move > typedefs out of typedefs.h: it slows down the build. Granted, the four (three - one added line is the typedef). > added by this patch are a drop in the bucket. The point I'm trying to > make is typedefs.h's purpose: it's for avoiding #include directives. > Circular ones in particular, but others, too. In this case, adding ssi.h inclusions to SSI controllers seems to be a feature, not a bug. Paolo
On 15/01/19 13:56, Thomas Huth wrote: > Yes, agreed, removing things from typedefs.h just to add lots of > #includes in other files is not really the best idea. I also dropped > this patch in v2 of my current PULL request because of this reason. > Phil, I suggest to simply drop this patch. What we maybe could do is to > split up typedefs.h per subsystem, so that we additionally have a > hw/arm/typedefs.h, hw/ppc/typedefs.h etc. in the end, then the > target-specific typedefs would not clutter the common qemu/typedefs.h > file anymore. I disagree. The *inclusions* of target-specific typedefs.h would then clutter something. For the (important, mind) case of circular includes, allowing struct in prototypes pretty much solves the issues, and a subsystem-specific typedefs.h is another solution according to maintainer's discretion. In this case, however, keeping subsystems self-contained is a good reason to apply the patch. Having SSIBus in typedefs.h means that the next SSI type has a higher chance of being added to typedefs.h even if it's not necessary. Sometimes we need to take patches that seem unnecessary in order to keep the codebase tidy. Think of the churn that was the introduction of hw/SUBSYSTEM. It was painful and it added all the complexity to the Makefiles in order to support recursive Makefile.objs in our not-that-recursive makefiles. However, it made MAINTAINERS usable and now, a few years later, few of us would probably be happy to go back to the flat hw/ directory. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 15/01/19 13:28, Markus Armbruster wrote: >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- >>> hw/arm/strongarm.h | 1 + >>> include/hw/arm/pxa.h | 1 + >>> include/hw/ssi/pl022.h | 1 + >>> include/hw/ssi/ssi.h | 1 + >>> include/qemu/typedefs.h | 1 - >>> 5 files changed, 4 insertions(+), 1 deletion(-) >> When typedefs.h changes, we recompile the world, but it pretty much only >> ever changes when new typedefs are added. Thus, *keeping* a typedef >> there is therefore pretty cheap. >> >> Nevertheless, we shouldn't keep typedefs there without a real reason. >> Being able to move one away without having to add any new #include >> directives is a strong sign for "no real reason". I like patches doing >> that. >> >> What I don't like is adding #include directives just so you can move >> typedefs out of typedefs.h: it slows down the build. Granted, the four > > (three - one added line is the typedef). Correct. >> added by this patch are a drop in the bucket. The point I'm trying to >> make is typedefs.h's purpose: it's for avoiding #include directives. >> Circular ones in particular, but others, too. > > In this case, adding ssi.h inclusions to SSI controllers seems to be a > feature, not a bug. Adding #include can be a necessity. It can't be a feature any more than "slowing down your compiles" could be one :) I'm particularly wary of unnecessary #include in headers.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 15/01/19 13:56, Thomas Huth wrote: >> Yes, agreed, removing things from typedefs.h just to add lots of >> #includes in other files is not really the best idea. I also dropped >> this patch in v2 of my current PULL request because of this reason. >> Phil, I suggest to simply drop this patch. What we maybe could do is to >> split up typedefs.h per subsystem, so that we additionally have a >> hw/arm/typedefs.h, hw/ppc/typedefs.h etc. in the end, then the >> target-specific typedefs would not clutter the common qemu/typedefs.h >> file anymore. > > I disagree. The *inclusions* of target-specific typedefs.h would then > clutter something. > > For the (important, mind) case of circular includes, allowing struct in > prototypes pretty much solves the issues, and a subsystem-specific > typedefs.h is another solution according to maintainer's discretion. > > In this case, however, keeping subsystems self-contained is a good > reason to apply the patch. Having SSIBus in typedefs.h means that the > next SSI type has a higher chance of being added to typedefs.h even if > it's not necessary. typedefs.h churn appars to be a non-problem: $ git-log --oneline --since 'one year ago' include/qemu/typedefs.h a98c370c46 typedefs: (Re-)sort entries alphabetically aec90730fb numa: Match struct to typedef name 7cfda775e5 move ObjectClass to typedefs.h ea134caa08 typedefs: add QJSON 201376cb9e typedefs: Remove PcGuestInfo from qemu/typedefs.h 9f5c734d59 Typedef the subtypes of QObject in qemu/typedefs.h, too e70372fcaf lockable: add QemuLockable > Sometimes we need to take patches that seem unnecessary in order to keep > the codebase tidy. Think of the churn that was the introduction of > hw/SUBSYSTEM. It was painful and it added all the complexity to the > Makefiles in order to support recursive Makefile.objs in our > not-that-recursive makefiles. However, it made MAINTAINERS usable and > now, a few years later, few of us would probably be happy to go back to > the flat hw/ directory. What problem exactly are we trying to solve here? To the best of my knowledge, typedefs.h has been working just fine for us, and I can't see why it shouldn't continue to work just fine.
On 16/01/19 09:33, Markus Armbruster wrote: > What problem exactly are we trying to solve here? > To the best of my knowledge, typedefs.h has been working just fine for > us, and I can't see why it shouldn't continue to work just fine. Patches don't have to solve problems. They can just help long-term maintainability, highlight code smells, etc. Nobody is saying typedefs.h doesn't work. But it's a tool, and including headers from headers is also a tool. Each tool has its purpose and it is useful to question what are the exact purposes of the tools. typedefs.h is useful to avoid rebuilding the world too often if a type is used many times as a pointer, but rarely as a struct and rarely has functions called on its instances. This is already a relatively rare case, but here we're talking about files that are included less than 20 times; many of which also already include hw/ssi/ssi.h for their own business. So we're hardly rebuilding anything more often, also because hw/ssi/ssi.h is not really changing fast. typedefs.h is also useful to avoid circular header inclusions. Here hw/ssi/ssi.h is clearly a toplevel include for the SSI subsystem. I agree that includes in a headers _can_ be painful, but sometimes they also tell you a hierarchy of what uses what. typedefs.h doesn't; forward struct declarations also do, and I all but dislike using them in headers (Thomas, can you send v2 of the HACKING patch?). Therefore, typedefs.h is not particularly useful for SSIBus. It doesn't hurt much, if at all, to leave it in. On the other hand, it also doesn't hurt much if at all to remove it; it makes the SSI code a very tiny little bit more self contained. It may be that it's not particularly useful just because SSI is small; for example I2CBus is also in typedefs.h and it's used a lot more, maybe one day SSIBus will be the same. In doubt, let's drop the patch---but I think it's useful to have the discussion and there are cases that are not controversial at all in Philippe's series. Paolo
Hi, > typedefs.h is useful to avoid rebuilding the world too often if a type > is used many times as a pointer, but rarely as a struct and rarely has > functions called on its instances. Related: Can also be used to keep struct content private. struct QemuConsole for example is private to ui/console.c, but pointers to QemuConsole are passed around alot in ui/* and hw/display/* code. cheers, Gerd
On 16/01/19 12:34, Gerd Hoffmann wrote: > Hi, > >> typedefs.h is useful to avoid rebuilding the world too often if a type >> is used many times as a pointer, but rarely as a struct and rarely has >> functions called on its instances. > > Related: Can also be used to keep struct content private. struct > QemuConsole for example is private to ui/console.c, but pointers to > QemuConsole are passed around alot in ui/* and hw/display/* code. True, though as we switch more and more from pointers to embedded structs that does not work that much anymore. Another way to do that is to split the header in include/path/to/foo.h and path/to/foo_internal.h. Paolo
On Wed, Jan 16, 2019 at 12:49:07PM +0100, Paolo Bonzini wrote: > On 16/01/19 12:34, Gerd Hoffmann wrote: > > Hi, > > > >> typedefs.h is useful to avoid rebuilding the world too often if a type > >> is used many times as a pointer, but rarely as a struct and rarely has > >> functions called on its instances. > > > > Related: Can also be used to keep struct content private. struct > > QemuConsole for example is private to ui/console.c, but pointers to > > QemuConsole are passed around alot in ui/* and hw/display/* code. > > True, though as we switch more and more from pointers to embedded > structs that does not work that much anymore. Another way to do that is > to split the header in include/path/to/foo.h and path/to/foo_internal.h. > > Paolo Not sure this will help since no tool checks structure isn't actually used even though it's in internal. If you want to go overboard it's solvable of course, e.g. something like this will work: E.g. in virtio.h #ifndef VIRTIO_PRIVATE #define VIRTIO_PRIVATE(f) (VIRTIO_PIVATE_##f) #endif struct VirtioPrivate { int VIRTIO_PRIVATE(bar); }; and in virtio.c: #define VIRTIO_PRIVATE(f) (f) #include <virtio.h>
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Jan 16, 2019 at 12:49:07PM +0100, Paolo Bonzini wrote: >> On 16/01/19 12:34, Gerd Hoffmann wrote: >> > Hi, >> > >> >> typedefs.h is useful to avoid rebuilding the world too often if a type >> >> is used many times as a pointer, but rarely as a struct and rarely has >> >> functions called on its instances. >> > >> > Related: Can also be used to keep struct content private. struct >> > QemuConsole for example is private to ui/console.c, but pointers to >> > QemuConsole are passed around alot in ui/* and hw/display/* code. >> >> True, though as we switch more and more from pointers to embedded >> structs that does not work that much anymore. Another way to do that is >> to split the header in include/path/to/foo.h and path/to/foo_internal.h. >> >> Paolo > > Not sure this will help since no tool checks structure isn't > actually used even though it's in internal. > > If you want to go overboard it's solvable of course, e.g. > something like this will work: > > > E.g. in virtio.h > > #ifndef VIRTIO_PRIVATE > #define VIRTIO_PRIVATE(f) (VIRTIO_PIVATE_##f) > #endif > > struct VirtioPrivate { > int VIRTIO_PRIVATE(bar); > }; > > and in virtio.c: > > #define VIRTIO_PRIVATE(f) (f) > #include <virtio.h> I don't like such games at all. Instead, I'd recommend to keep internal headers out of include/, and rely on review / grep to catch, correct and prevent inappropriate uses.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 16/01/19 09:33, Markus Armbruster wrote: >> What problem exactly are we trying to solve here? >> To the best of my knowledge, typedefs.h has been working just fine for >> us, and I can't see why it shouldn't continue to work just fine. > > Patches don't have to solve problems. They can just help long-term > maintainability, highlight code smells, etc. Nobody is saying > typedefs.h doesn't work. But it's a tool, and including headers from > headers is also a tool. Each tool has its purpose and it is useful to > question what are the exact purposes of the tools. > > typedefs.h is useful to avoid rebuilding the world too often if a type > is used many times as a pointer, but rarely as a struct and rarely has > functions called on its instances. This is already a relatively rare > case, but here we're talking about files that are included less than 20 > times; many of which also already include hw/ssi/ssi.h for their own > business. So we're hardly rebuilding anything more often, also because > hw/ssi/ssi.h is not really changing fast. > > typedefs.h is also useful to avoid circular header inclusions. Here > hw/ssi/ssi.h is clearly a toplevel include for the SSI subsystem. I > agree that includes in a headers _can_ be painful, but sometimes they > also tell you a hierarchy of what uses what. typedefs.h doesn't; > forward struct declarations also do, and I all but dislike using them in > headers (Thomas, can you send v2 of the HACKING patch?). > > Therefore, typedefs.h is not particularly useful for SSIBus. It doesn't > hurt much, if at all, to leave it in. On the other hand, it also > doesn't hurt much if at all to remove it; it makes the SSI code a very > tiny little bit more self contained. > > It may be that it's not particularly useful just because SSI is small; > for example I2CBus is also in typedefs.h and it's used a lot more, maybe > one day SSIBus will be the same. In doubt, let's drop the patch---but I > think it's useful to have the discussion and there are cases that are > not controversial at all in Philippe's series. I fully agree having the discussion is useful, and I also like many of the patches in Philippe's series.
diff --git a/hw/arm/strongarm.h b/hw/arm/strongarm.h index e98840b461..ae51a1ae34 100644 --- a/hw/arm/strongarm.h +++ b/hw/arm/strongarm.h @@ -3,6 +3,7 @@ #include "exec/memory.h" #include "target/arm/cpu-qom.h" +#include "hw/ssi/ssi.h" #define SA_CS0 0x00000000 #define SA_CS1 0x08000000 diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h index f6dfb5c0cf..f184349f02 100644 --- a/include/hw/arm/pxa.h +++ b/include/hw/arm/pxa.h @@ -13,6 +13,7 @@ #include "exec/memory.h" #include "target/arm/cpu-qom.h" #include "hw/pcmcia.h" +#include "hw/ssi/ssi.h" /* Interrupt numbers */ # define PXA2XX_PIC_SSP3 0 diff --git a/include/hw/ssi/pl022.h b/include/hw/ssi/pl022.h index a080519366..1cf16f1539 100644 --- a/include/hw/ssi/pl022.h +++ b/include/hw/ssi/pl022.h @@ -22,6 +22,7 @@ #define HW_SSI_PL022_H #include "hw/sysbus.h" +#include "hw/ssi/ssi.h" #define TYPE_PL022 "pl022" #define PL022(obj) OBJECT_CHECK(PL022State, (obj), TYPE_PL022) diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h index 6a0c3c3cdb..bdbf3c51f5 100644 --- a/include/hw/ssi/ssi.h +++ b/include/hw/ssi/ssi.h @@ -13,6 +13,7 @@ #include "hw/qdev.h" +typedef struct SSIBus SSIBus; typedef struct SSISlave SSISlave; typedef struct SSISlaveClass SSISlaveClass; typedef enum SSICSMode SSICSMode; diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h index 3bd9215d55..c026229573 100644 --- a/include/qemu/typedefs.h +++ b/include/qemu/typedefs.h @@ -108,7 +108,6 @@ typedef struct Range Range; typedef struct SerialState SerialState; typedef struct SHPCDevice SHPCDevice; typedef struct SMBusDevice SMBusDevice; -typedef struct SSIBus SSIBus; typedef struct uWireSlave uWireSlave; typedef struct VirtIODevice VirtIODevice; typedef struct Visitor Visitor;