Re: [Qemu-devel] [PATCH v4 2/3] ppc: Drop duplicated typedefs to be able to compile with Clang in gnu99 mode

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5167 bytes --]

On Thu, Jan 10, 2019 at 03:07:59PM +0100, Thomas Huth wrote:
> On 2019-01-10 14:15, Greg Kurz wrote:
> > On Thu, 10 Jan 2019 10:15:35 +0100
> > Thomas Huth <thuth@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...)

I'd say it's definitely qemu coding style in practice, whatever you
argue about the wording in the docs.

> 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
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]