| Message ID | 20181113165247.4806-3-sameo@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support disabling TCG on ARM | expand |
On 13/11/18 18:01, Peter Maydell wrote: > On 13 November 2018 at 16:52, Samuel Ortiz <sameo@linux.intel.com> wrote: >> From: Philippe Mathieu-Daudé <philmd@redhat.com> >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> Reviewed-by: Robert Bradford <robert.bradford@intel.com> >> Reviewed-by: Samuel Ortiz <sameo@linux.intel.com> >> --- >> target/arm/helper.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/target/arm/helper.c b/target/arm/helper.c >> index 3d4e9c5c8a..27d9285e1e 100644 >> --- a/target/arm/helper.c >> +++ b/target/arm/helper.c >> @@ -12,13 +12,10 @@ >> #include "internals.h" >> #include "exec/gdbstub.h" >> #include "exec/helper-proto.h" >> -#include "qemu/host-utils.h" > > This is for muldiv64(). But it is already included by "cpu.h" -> "exec/cpu-defs.h" > >> #include "sysemu/arch_init.h" >> #include "sysemu/sysemu.h" >> -#include "qemu/bitops.h" "cpu.h" -> "cpu-qom.h" -> "qom/cpu.h" -> "qemu/bitmap.h" > > This is for extract32() and friends. > >> #include "qemu/crc32c.h" >> #include "exec/exec-all.h" >> -#include "exec/cpu_ldst.h" > > This is for cpu_stl_data(). Included by "arm_ldst.h" > >> #include "arm_ldst.h" >> #include <zlib.h> /* For crc32 */ >> #include "exec/semihost.h" So they are not "unused" but "unnecessary". I thought this would be better to clean this once, before Samuel split. Samuel: please drop this patch from your series. Thanks, Phil.
On 13 November 2018 at 18:02, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 13/11/18 18:01, Peter Maydell wrote: >> >> On 13 November 2018 at 16:52, Samuel Ortiz <sameo@linux.intel.com> wrote: >>> --- a/target/arm/helper.c >>> +++ b/target/arm/helper.c >>> @@ -12,13 +12,10 @@ >>> #include "internals.h" >>> #include "exec/gdbstub.h" >>> #include "exec/helper-proto.h" >>> -#include "qemu/host-utils.h" >> >> >> This is for muldiv64(). > > > But it is already included by "cpu.h" -> "exec/cpu-defs.h" > So they are not "unused" but "unnecessary". > > I thought this would be better to clean this once, before Samuel split. Generally I think that if a .c file directly uses function X declared in header Y it should #include Y, even if it happens that it already includes other header Z that includes Y. Otherwise if we refactor Z later such that it no longer needs to include Y, it will break compilation of the .c file. (That is, Z including Y is a detail of the implementation of Z, not a guarantee made by Z to its users.) The exception here is where the header guarantees that it's going to include certain other things (which is the case for eg our osdep.h). thanks -- PMM
On Tue, Nov 13, 2018 at 7:08 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On 13 November 2018 at 18:02, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 13/11/18 18:01, Peter Maydell wrote: > >> On 13 November 2018 at 16:52, Samuel Ortiz <sameo@linux.intel.com> wrote: > > >>> --- a/target/arm/helper.c > >>> +++ b/target/arm/helper.c > >>> @@ -12,13 +12,10 @@ > >>> #include "internals.h" > >>> #include "exec/gdbstub.h" > >>> #include "exec/helper-proto.h" > >>> -#include "qemu/host-utils.h" > >> > >> > >> This is for muldiv64(). > > > > > > But it is already included by "cpu.h" -> "exec/cpu-defs.h" > > > So they are not "unused" but "unnecessary". > > > > I thought this would be better to clean this once, before Samuel split. > > Generally I think that if a .c file directly uses function X declared in > header Y it should #include Y, even if it happens that it already includes > other header Z that includes Y. Otherwise if we refactor Z later such > that it no longer needs to include Y, it will break compilation of the .c > file. (That is, Z including Y is a detail of the implementation of Z, > not a guarantee made by Z to its users.) Yes, this makes sense now, thanks. Phil. > The exception here is where the header guarantees that it's going > to include certain other things (which is the case for eg our osdep.h). > > thanks > -- PMM
Hi Philippe, On Tue, Nov 13, 2018 at 07:02:57PM +0100, Philippe Mathieu-Daudé wrote: > On 13/11/18 18:01, Peter Maydell wrote: > > On 13 November 2018 at 16:52, Samuel Ortiz <sameo@linux.intel.com> wrote: > > > From: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > Reviewed-by: Robert Bradford <robert.bradford@intel.com> > > > Reviewed-by: Samuel Ortiz <sameo@linux.intel.com> > > > --- > > > target/arm/helper.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c > > > index 3d4e9c5c8a..27d9285e1e 100644 > > > --- a/target/arm/helper.c > > > +++ b/target/arm/helper.c > > > @@ -12,13 +12,10 @@ > > > #include "internals.h" > > > #include "exec/gdbstub.h" > > > #include "exec/helper-proto.h" > > > -#include "qemu/host-utils.h" > > > > This is for muldiv64(). > > But it is already included by "cpu.h" -> "exec/cpu-defs.h" > > > > > > #include "sysemu/arch_init.h" > > > #include "sysemu/sysemu.h" > > > -#include "qemu/bitops.h" > > "cpu.h" -> "cpu-qom.h" -> "qom/cpu.h" -> "qemu/bitmap.h" > > > > > This is for extract32() and friends. > > > > > #include "qemu/crc32c.h" > > > #include "exec/exec-all.h" > > > -#include "exec/cpu_ldst.h" > > > > This is for cpu_stl_data(). > > Included by "arm_ldst.h" > > > > > > #include "arm_ldst.h" > > > #include <zlib.h> /* For crc32 */ > > > #include "exec/semihost.h" > > So they are not "unused" but "unnecessary". > > I thought this would be better to clean this once, before Samuel split. > > Samuel: please drop this patch from your series. Dropped. I updated my https://github.com/intel/nemu/tree/topic/upstream/arm-tcg-disable branch accordingly. I will wait for Richard's review before sending a v2. Cheers, Samuel.
diff --git a/target/arm/helper.c b/target/arm/helper.c index 3d4e9c5c8a..27d9285e1e 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -12,13 +12,10 @@ #include "internals.h" #include "exec/gdbstub.h" #include "exec/helper-proto.h" -#include "qemu/host-utils.h" #include "sysemu/arch_init.h" #include "sysemu/sysemu.h" -#include "qemu/bitops.h" #include "qemu/crc32c.h" #include "exec/exec-all.h" -#include "exec/cpu_ldst.h" #include "arm_ldst.h" #include <zlib.h> /* For crc32 */ #include "exec/semihost.h"