On Thu, 10 Jan 2019 15:07:59 +0100 Thomas Huth <th...@redhat.com> wrote:
> On 2019-01-10 14:15, Greg Kurz wrote: > > On Thu, 10 Jan 2019 10:15:35 +0100 > > Thomas Huth <th...@redhat.com> wrote: > > > >> When compiling the ppc code with clang and -std=gnu99, there are a > >> couple of warnings/errors like this one: > >> > >> CC ppc64-softmmu/hw/intc/xics.o > >> In file included from /home/thuth/devel/qemu/hw/intc/xics.c:35: > >> /home/thuth/devel/qemu/include/hw/ppc/xics.h:43:25: error: redefinition of > >> typedef 'ICPState' is a C11 feature > >> [-Werror,-Wtypedef-redefinition] > >> typedef struct ICPState ICPState; > >> ^ > >> /home/thuth/devel/qemu/target/ppc/cpu.h:1181:25: note: previous definition > >> is here > >> typedef struct ICPState ICPState; > >> ^ > >> > >> Drop the duplicated typedefs and use normal "struct" forward declarations > >> like we already do it at the top of spapr.h for a couple of other > >> definitions. > >> > > > > Hmm... so the choice here is to simply ignore the official coding > > style ? > > Are typedefs really our "official coding style"? It's mentioned in > HACKING, not in CODING_STYLE, so I rather see this as a recommendation Indeed. > only. (Otherwise, all the forward struct definitions at the beginning of > spapr.h are a plain violation of the coding style, too...) > Yeah. > IMHO we should rather adopt the coding style of the kernel which rather > tries to avoid to typedef each and every struct. > From https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs : "In general, a pointer, or a struct that has elements that can reasonably be directly accessed should never be a typedef." So if we were to adopt the coding style of the kernel, we'd have to face a lot more violations with the current QEMU code base :) > > It is a bit confusing to end up with even more struct/non-struct > > inconsistency. It would be good at least to update HACKING so that > > people know when they can legitimately do that... or we simply don't > > care anymore for the typedef rule ? > > We should maybe limit the recommendation for the typedefs to things that > we mainly need in common code and that also fit into > include/qemu/typedefs.h nicely. If we agree on that, I could send an > update for the HACKING file. > Makes sense. > > All these forward declarations could be typedefs in a "hw/ppc/spapr_types.h" > > header as well, as suggested elsewhere by Daniel. > > I'd prefer to rather get rid of the typedefs in this case instead of > introducing spapr_types.h ... but if other ppc folks are also keen on > that file (David?), I can rework my patch to introduce it. > > >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > >> index 9e01a5a..10d069e 100644 > >> --- a/include/hw/ppc/spapr.h > >> +++ b/include/hw/ppc/spapr.h > >> @@ -12,11 +12,12 @@ > >> struct VIOsPAPRBus; > >> struct sPAPRPHBState; > >> struct sPAPRNVRAM; > >> +struct ICSState; > >> +struct sPAPRXive; > >> + > >> typedef struct sPAPREventLogEntry sPAPREventLogEntry; > >> typedef struct sPAPREventSource sPAPREventSource; > >> typedef struct sPAPRPendingHPT sPAPRPendingHPT; > >> -typedef struct ICSState ICSState; > > > > Thanks to the previous patch, I guess the ICSState type could be > > obtained by including "hw/ppc/xics.h". > > There are already plenty of other struct forward declarations without > typedefs here, so I assume my changes are ok here. David? > > >> -typedef struct sPAPRXive sPAPRXive; > >> > >> #define HPTE64_V_HPTE_DIRTY 0x0000000000000040ULL > >> #define SPAPR_ENTRY_POINT 0x100 > >> @@ -127,7 +128,7 @@ struct sPAPRMachineState { > >> struct VIOsPAPRBus *vio_bus; > >> QLIST_HEAD(, sPAPRPHBState) phbs; > >> struct sPAPRNVRAM *nvram; > >> - ICSState *ics; > >> + struct ICSState *ics; > >> sPAPRRTCState rtc; > >> > >> sPAPRResizeHPT resize_hpt; > >> @@ -180,7 +181,7 @@ struct sPAPRMachineState { > >> const char *icp_type; > >> int32_t irq_map_nr; > >> unsigned long *irq_map; > >> - sPAPRXive *xive; > >> + struct sPAPRXive *xive; > >> sPAPRIrq *irq; > >> qemu_irq *qirqs; > [...] > >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > >> index 486abaf..a62ff60 100644 > >> --- a/target/ppc/cpu.h > >> +++ b/target/ppc/cpu.h > >> @@ -1177,8 +1177,9 @@ do { \ > >> > >> typedef struct PPCVirtualHypervisor PPCVirtualHypervisor; > >> typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass; > >> -typedef struct XiveTCTX XiveTCTX; > >> -typedef struct ICPState ICPState; > >> + > >> +struct XiveTCTX; > >> +struct ICPState; > > > > These could be made available from the XICS/XIVE header files. > > > > #ifndef CONFIG_USER_ONLY > > #include "hw/ppc/xive.h" /* for XiveTCTX */ > > #include "hw/ppc/xics.h" /* for ICPState */ > > #endif > > Ok, I can change it if we agree that normal struct forward declarations > are a no-go. Otherwise, I'd prefer the non-typedeffed struct forward > declarations here, I think. > Sure. > Thomas
