Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"

From: Paolo Bonzini <pbonzini@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: "Thomas Huth" <thuth@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Peter Crosthwaite" <crosthwaite.peter@gmail.com>,
	"Xiao Guangrong" <xiaoguangrong.eric@gmail.com>,
	qemu-block@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>,
	"Alistair Francis" <alistair@alistair23.me>,
	qemu-devel@nongnu.org, "Philippe Mathieu-Daudé" <f4bug@amsat.org>,
	"Greg Kurz" <groug@kaod.org>,
	qemu-arm@nongnu.org, "Gerd Hoffmann" <kraxel@redhat.com>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Laszlo Ersek" <lersek@redhat.com>,
	"Cédric Le Goater" <clg@kaod.org>
Subject: Re: [Qemu-devel] [PATCH 03/15] hw/ssi: Remove SSIBus from "qemu/typedefs.h"
Date: Wed, 16 Jan 2019 11:03:15 +0100	[thread overview]
Message-ID: <4b24723e-5a27-f25f-76c1-f2f4e94757ba@redhat.com> (raw)
In-Reply-To: <87sgxtatod.fsf@dusky.pond.sub.org>

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