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
only. (Otherwise, all the forward struct definitions at the beginning of
spapr.h are a plain violation of the coding style, too...)
IMHO we should rather adopt the coding style of the kernel which rather
tries to avoid to typedef each and every struct.
> 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.
> 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.
Thomas
