Re: [PATCH v3 1/3] hw/core: Cleanup unused included headers in cpu-common.c

[Philippe Mathieu-Daudé]

On 11/3/24 16:44, Zhao Liu wrote:
> Hi Philippe,
>
> On Mon, Mar 11, 2024 at 12:40:23PM +0100, Philippe Mathieu-Daudé wrote:
> > Date: Mon, 11 Mar 2024 12:40:23 +0100
> > From: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Subject: Re: [PATCH v3 1/3] hw/core: Cleanup unused included headers in
> >   cpu-common.c
> >
> > On 11/3/24 08:56, Zhao Liu wrote:
> > > From: Zhao Liu <zhao1.liu@intel.com>
> > >
> > > Remove unused headers in cpu-common.c:
> > > * qemu/notify.h
> > > * exec/cpu-common.h
> > > * qemu/error-report.h
> > > * qemu/qemu-print.h
> > >
> > > Tested by "./configure" and then "make".
> >
> > This isn't often enough. The safest way to catch implicit
> > includes is to add #error in them and compile the source.
> >
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > ---
> > >    hw/core/cpu-common.c | 4 ----
> > >    1 file changed, 4 deletions(-)
> > >
> > > diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> > > index 0108fb11dbc8..4bd9c70a83f1 100644
> > > --- a/hw/core/cpu-common.c
> > > +++ b/hw/core/cpu-common.c
> > > @@ -22,14 +22,10 @@
> > >    #include "qapi/error.h"
> > >    #include "hw/core/cpu.h"
> > >    #include "sysemu/hw_accel.h"
> > > -#include "qemu/notify.h"
> > >    #include "qemu/log.h"
> > >    #include "qemu/main-loop.h"
> > >    #include "exec/log.h"
> > > -#include "exec/cpu-common.h"
> >
> > Watch out, "exec/cpu-common.h" is implicitly included:
> >
> > $ git diff -U0
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index 6346df17ce..27961bacc6 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -2,0 +3 @@
> > +#error
> >
> > $ ninja libcommon.fa.p/hw_core_cpu-common.c.o
> > In file included from ../../hw/core/cpu-common.c:34:
> > In file included from include/hw/boards.h:6:
> > In file included from include/exec/memory.h:19:
> > include/exec/cpu-common.h:3:2: error:
> > #error
>
> Thanks for helpping me verify this!!
>
> EMM, but I'm still not understanding how this approach distinguishes
> whether hw/core/cpu-common.c needs the header (include/exec/cpu-common.h)
> directly or just include/exec/memory.h needs that header? For the latter,
> the header needn't be included in .c file.

Yes, you are right, it might not be necessary.

For all other headers in your series I checked that no function /
definition is used in the source, but "exec/cpu-common.h" is too
big to do that manually. I mostly skipped it because it is odd to
have cpu-common.c not including the header with the same name...
Also, in another series I split / reworked "exec/cpu-common.h" and
IIRC a part of it will be included here. Bah, I'll stop writing
and take your patch unmodified.