[PATCH for-7.0 0/4] qemu-common.h include cleanup

Peter Maydell posted 4 patches 2 years, 5 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211129200510.1233037-1-peter.maydell@linaro.org
Maintainers: Yoshinori Sato <ysato@users.sourceforge.jp>, Leif Lindholm <leif@nuviainc.com>, Taylor Simpson <tsimpson@quicinc.com>, Radoslaw Biernacki <rad@semihalf.com>, "Michael S. Tsirkin" <mst@redhat.com>, Laurent Vivier <laurent@vivier.eu>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Havard Skinnemoen <hskinnemoen@google.com>, Sergio Lopez <slp@redhat.com>, Tyrone Ting <kfting@nuvoton.com>, Peter Maydell <peter.maydell@linaro.org>, Alistair Francis <alistair@alistair23.me>, Rob Herring <robh@kernel.org>, Paolo Bonzini <pbonzini@redhat.com>, Antony Pavlov <antonynpavlov@gmail.com>
include/hw/i386/microvm.h     | 1 -
include/hw/i386/x86.h         | 1 -
target/hexagon/cpu.h          | 1 -
target/rx/cpu.h               | 1 -
hw/arm/boot.c                 | 1 -
hw/arm/digic_boards.c         | 1 -
hw/arm/highbank.c             | 1 -
hw/arm/npcm7xx_boards.c       | 1 -
hw/arm/sbsa-ref.c             | 1 -
hw/arm/stm32f405_soc.c        | 1 -
hw/arm/vexpress.c             | 1 -
hw/arm/virt.c                 | 1 -
linux-user/hexagon/cpu_loop.c | 1 +
13 files changed, 1 insertion(+), 12 deletions(-)
[PATCH for-7.0 0/4] qemu-common.h include cleanup
Posted by Peter Maydell 2 years, 5 months ago
qemu-common.h has a comment at the top:

 * This file is supposed to be included only by .c files. No header file should
 * depend on qemu-common.h, as this would easily lead to circular header
 * dependencies.

We still have a few .h files which include it, though.  The first 3
patches in this series fix that: in 3 out of 4 cases we didn't need
the #include at all, and in the 4th case we can instead #include
qemu-common.h from just one .c file.

Patch 4 is just removing the #include from 8 files in hw/arm which
don't need it at all.  (Probably there are other files like this, but
I just did the Arm related ones.)

Tested by pushing to gitlab for the CI build.

-- PMM

Peter Maydell (4):
  include/hw/i386: Don't include qemu-common.h in .h files
  target/hexagon/cpu.h: don't include qemu-common.h
  target/rx/cpu.h: Don't include qemu-common.h
  hw/arm: Don't include qemu-common.h unnecessarily

 include/hw/i386/microvm.h     | 1 -
 include/hw/i386/x86.h         | 1 -
 target/hexagon/cpu.h          | 1 -
 target/rx/cpu.h               | 1 -
 hw/arm/boot.c                 | 1 -
 hw/arm/digic_boards.c         | 1 -
 hw/arm/highbank.c             | 1 -
 hw/arm/npcm7xx_boards.c       | 1 -
 hw/arm/sbsa-ref.c             | 1 -
 hw/arm/stm32f405_soc.c        | 1 -
 hw/arm/vexpress.c             | 1 -
 hw/arm/virt.c                 | 1 -
 linux-user/hexagon/cpu_loop.c | 1 +
 13 files changed, 1 insertion(+), 12 deletions(-)

-- 
2.25.1


Re: [PATCH for-7.0 0/4] qemu-common.h include cleanup
Posted by Richard Henderson 2 years, 5 months ago
On 11/29/21 9:05 PM, Peter Maydell wrote:
> qemu-common.h has a comment at the top:
> 
>   * This file is supposed to be included only by .c files. No header file should
>   * depend on qemu-common.h, as this would easily lead to circular header
>   * dependencies.
> 
> We still have a few .h files which include it, though.  The first 3
> patches in this series fix that: in 3 out of 4 cases we didn't need
> the #include at all, and in the 4th case we can instead #include
> qemu-common.h from just one .c file.
> 
> Patch 4 is just removing the #include from 8 files in hw/arm which
> don't need it at all.  (Probably there are other files like this, but
> I just did the Arm related ones.)
> 
> Tested by pushing to gitlab for the CI build.
> 
> -- PMM
> 
> Peter Maydell (4):
>    include/hw/i386: Don't include qemu-common.h in .h files
>    target/hexagon/cpu.h: don't include qemu-common.h
>    target/rx/cpu.h: Don't include qemu-common.h
>    hw/arm: Don't include qemu-common.h unnecessarily

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH for-7.0 0/4] qemu-common.h include cleanup
Posted by Peter Maydell 2 years, 5 months ago
On Mon, 29 Nov 2021 at 20:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> qemu-common.h has a comment at the top:
>
>  * This file is supposed to be included only by .c files. No header file should
>  * depend on qemu-common.h, as this would easily lead to circular header
>  * dependencies.

As a side note, that comment was added back in 2012 when qemu-common.h
was bigger, included other headers, and did some of the work we currently
use osdep.h for. As it stands today qemu-common.h includes no other
files so it isn't a source of possible circular dependencies -- it's
just a grab-bag of miscellaneous prototypes that in an ideal world
would be in more focused individual headers[*]. So there's an argument
for deleting this comment...

[*] A cleanup that would be nice, and I'm about to send out a patchset
that splits out the rtc related functions; but the grab-bag at the
bottom of osdep.h is probably higher priority because that header
gets pulled in by an order of magnitude more C files.

-- PMM

Re: [PATCH for-7.0 0/4] qemu-common.h include cleanup
Posted by Markus Armbruster 2 years, 5 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 29 Nov 2021 at 20:05, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> qemu-common.h has a comment at the top:
>>
>>  * This file is supposed to be included only by .c files. No header file should
>>  * depend on qemu-common.h, as this would easily lead to circular header
>>  * dependencies.
>
> As a side note, that comment was added back in 2012 when qemu-common.h
> was bigger, included other headers, and did some of the work we currently
> use osdep.h for. As it stands today qemu-common.h includes no other
> files so it isn't a source of possible circular dependencies -- it's
> just a grab-bag of miscellaneous prototypes that in an ideal world
> would be in more focused individual headers[*]. So there's an argument
> for deleting this comment...

First, thank you for this cleanup series.

The comment is from commit 04509ad939a:

    qemu-common.h: Comment about usage rules
    
    Every time we make a tiny change on a header file, we often find
    circular header dependency problems. To avoid this nightmare, we need to
    stop including qemu-common.h from other headers, and we should gradually
    move the declarations from the catch-all qemu-common.h header to their
    specific headers.
    
    This simply adds a comment documenting the rules about qemu-common.h,
    hoping that people will see it before including qemu-common.h from other
    header files, and before adding more declarations to qemu-common.h.
    
    Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

You're right: we've since moved all #include out of qemu-common.h, so
the risk of circular header dependency is gone, and the comment's claim
"this would easily lead to circular header dependencies" is no longer
correct.

In my opinion, including qemu-common.h in a header is a bad idea
regardless.  Headers should include the headers they need to make them
self-contained, and no more, because more risks slower compiles, in
particular frequent recompiles of pretty much everything.  Previously
discussed at

    https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg06291.html
    Message-ID: <877eac82il.fsf@dusky.pond.sub.org>

Nothing in qemu-common.h should be required to make another header
self-contained.

This is likely the case for other headers as well, which don't carry a
comment forbidding inclusion into headers.  You could argue that
qemu-common.h should not carry one, either.  I can accept that.  I'm not
attached to the comment.  I am interested in keeping unwanted #include
in headers under control.

> [*] A cleanup that would be nice, and I'm about to send out a patchset
> that splits out the rtc related functions; but the grab-bag at the
> bottom of osdep.h is probably higher priority because that header
> gets pulled in by an order of magnitude more C files.

By all "normal" ones, in fact.


Re: [PATCH for-7.0 0/4] qemu-common.h include cleanup
Posted by Philippe Mathieu-Daudé 2 years, 5 months ago
On 11/29/21 21:05, Peter Maydell wrote:

> Peter Maydell (4):
>   include/hw/i386: Don't include qemu-common.h in .h files
>   target/hexagon/cpu.h: don't include qemu-common.h
>   target/rx/cpu.h: Don't include qemu-common.h
>   hw/arm: Don't include qemu-common.h unnecessarily

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>