Re: [Qemu-devel] [PATCH 1/4] memory: make memory API parsable by gtkdoc-scan

[Anthony Liguori]

On 12/14/2011 10:34 AM, malc wrote:
> On Wed, 14 Dec 2011, Anthony Liguori wrote:
>
> > GTK/glib uses a convenient of:
> >
> >      typedef struct _CamelCase CamelCase;
> >
> > The reason that they use a separate struct name is that in C++, the struct
> > namespace not a separate namespace from the type namespace.  This is actually a
> > reasonable policy for QEMU to adopt as we eventually start exporting C libraries
> > that may be consumed by C++ programs.
> >
> > I think the use of _ does not violate the C specification as the struct
> > namespace is not the same as the type namespace which is what the C spec refers
> > to if I understand it correctly.
>
> It does violate the standard _ followed by upper case letter is reserved
> in all contexts.

I think in this case, we're just going to have to deviate from the standard. 
The benefit is great, the practice is widespread, and there's no immediate 
likelihood of changing glib's coding style practices.

I still think we should adhere to this policy where ever possible but I think 
we'll have to make an exception for extracting documentation.

Regards,

Anthony Liguori

> > Additionally, gtkdoc-scan cannot handle nested structs so remove those from the
> > memory API.
> >
> > Signed-off-by: Anthony Liguori<address@hidden>
> > ---
> >   ioport.h |   14 ++++----
> >   memory.c |    6 ++--
> >   memory.h |   99 +++++++++++++++++++++++++++++++++----------------------------
> >   3 files changed, 64 insertions(+), 55 deletions(-)
> >
> > diff --git a/ioport.h b/ioport.h
> > index ae3e9da..99345d8 100644
> > --- a/ioport.h
> > +++ b/ioport.h
> > @@ -52,24 +52,24 @@ uint8_t cpu_inb(pio_addr_t addr);
> >   uint16_t cpu_inw(pio_addr_t addr);
> >   uint32_t cpu_inl(pio_addr_t addr);
> >
> > -struct MemoryRegion;
> > -struct MemoryRegionPortio;
> > +struct _MemoryRegion;
> > +struct _MemoryRegionPortio;
> >
> >   typedef struct PortioList {
> > -    const struct MemoryRegionPortio *ports;
> > -    struct MemoryRegion *address_space;
> > +    const struct _MemoryRegionPortio *ports;
> > +    struct _MemoryRegion *address_space;
> >       unsigned nr;
> > -    struct MemoryRegion **regions;
> > +    struct _MemoryRegion **regions;
> >       void *opaque;
> >       const char *name;
> >   } PortioList;
> >
> >   void portio_list_init(PortioList *piolist,
> > -                      const struct MemoryRegionPortio *callbacks,
> > +                      const struct _MemoryRegionPortio *callbacks,
> >                         void *opaque, const char *name);
> >   void portio_list_destroy(PortioList *piolist);
> >   void portio_list_add(PortioList *piolist,
> > -                     struct MemoryRegion *address_space,
> > +                     struct _MemoryRegion *address_space,
> >                        uint32_t addr);
> >   void portio_list_del(PortioList *piolist);
> >
> > diff --git a/memory.c b/memory.c
> > index adfdf14..76a7ae6 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -72,12 +72,12 @@ static AddrRange addrrange_intersection(AddrRange r1, 
> > AddrRange r2)
> >       return addrrange_make(start, int128_sub(end, start));
> >   }
> >
> > -struct CoalescedMemoryRange {
> > +struct _CoalescedMemoryRange {
> >       AddrRange addr;
> > -    QTAILQ_ENTRY(CoalescedMemoryRange) link;
> > +    QTAILQ_ENTRY(_CoalescedMemoryRange) link;
> >   };
> >
> > -struct MemoryRegionIoeventfd {
> > +struct _MemoryRegionIoeventfd {
> >       AddrRange addr;
> >       bool match_data;
> >       uint64_t data;
> > diff --git a/memory.h b/memory.h
> > index beae127..3aa8404 100644
> > --- a/memory.h
> > +++ b/memory.h
> > @@ -26,10 +26,12 @@
> >   #include "ioport.h"
> >   #include "int128.h"
> >
> > -typedef struct MemoryRegionOps MemoryRegionOps;
> > -typedef struct MemoryRegion MemoryRegion;
> > -typedef struct MemoryRegionPortio MemoryRegionPortio;
> > -typedef struct MemoryRegionMmio MemoryRegionMmio;
> > +typedef struct _MemoryRegionOps MemoryRegionOps;
> > +typedef struct _MemoryRegion MemoryRegion;
> > +typedef struct _MemoryRegionPortio MemoryRegionPortio;
> > +typedef struct _MemoryRegionMmio MemoryRegionMmio;
> > +typedef struct _MemoryRegionGuestConstraints MemoryRegionGuestConstraints;
> > +typedef struct _MemoryRegionInternalConstraints 
> > MemoryRegionInternalConstraints;
> >
> >   /* Must match *_DIRTY_FLAGS in cpu-all.h.  To be replaced with dynamic
> >    * registration.
> > @@ -38,15 +40,51 @@ typedef struct MemoryRegionMmio MemoryRegionMmio;
> >   #define DIRTY_MEMORY_CODE      1
> >   #define DIRTY_MEMORY_MIGRATION 3
> >
> > -struct MemoryRegionMmio {
> > +struct _MemoryRegionMmio {
> >       CPUReadMemoryFunc *read[3];
> >       CPUWriteMemoryFunc *write[3];
> >   };
> >
> > +struct _MemoryRegionGuestConstraints
> > +{
> > +    /* If nonzero, specify bounds on access sizes beyond which a machine
> > +     * check is thrown.
> > +     */
> > +    unsigned min_access_size;
> > +    unsigned max_access_size;
> > +    /* If true, unaligned accesses are supported.  Otherwise unaligned
> > +     * accesses throw machine checks.
> > +     */
> > +    bool unaligned;
> > +    /*
> > +     * If present, and returns #false, the transaction is not accepted
> > +     * by the device (and results in machine dependent behaviour such
> > +     * as a machine check exception).
> > +     */
> > +    bool (*accepts)(void *opaque, target_phys_addr_t addr,
> > +                    unsigned size, bool is_write);
> > +};
> > +
> > +struct _MemoryRegionInternalConstraints
> > +{
> > +    /* If nonzero, specifies the minimum size implemented.  Smaller sizes
> > +     * will be rounded upwards and a partial result will be returned.
> > +     */
> > +    unsigned min_access_size;
> > +    /* If nonzero, specifies the maximum size implemented.  Larger sizes
> > +     * will be done as a series of accesses with smaller sizes.
> > +     */
> > +    unsigned max_access_size;
> > +    /* If true, unaligned accesses are supported.  Otherwise all accesses
> > +     * are converted to (possibly multiple) naturally aligned accesses.
> > +     */
> > +    bool unaligned;
> > +};
> > +
> >   /*
> >    * Memory region callbacks
> >    */
> > -struct MemoryRegionOps {
> > +struct _MemoryRegionOps {
> >       /* Read from the memory region. @addr is relative to @mr; @size is
> >        * in bytes. */
> >       uint64_t (*read)(void *opaque,
> > @@ -61,39 +99,10 @@ struct MemoryRegionOps {
> >
> >       enum device_endian endianness;
> >       /* Guest-visible constraints: */
> > -    struct {
> > -        /* If nonzero, specify bounds on access sizes beyond which a machine
> > -         * check is thrown.
> > -         */
> > -        unsigned min_access_size;
> > -        unsigned max_access_size;
> > -        /* If true, unaligned accesses are supported.  Otherwise unaligned
> > -         * accesses throw machine checks.
> > -         */
> > -         bool unaligned;
> > -        /*
> > -         * If present, and returns #false, the transaction is not accepted
> > -         * by the device (and results in machine dependent behaviour such
> > -         * as a machine check exception).
> > -         */
> > -        bool (*accepts)(void *opaque, target_phys_addr_t addr,
> > -                        unsigned size, bool is_write);
> > -    } valid;
> > +    MemoryRegionGuestConstraints valid;
> > +
> >       /* Internal implementation constraints: */
> > -    struct {
> > -        /* If nonzero, specifies the minimum size implemented.  Smaller sizes
> > -         * will be rounded upwards and a partial result will be returned.
> > -         */
> > -        unsigned min_access_size;
> > -        /* If nonzero, specifies the maximum size implemented.  Larger sizes
> > -         * will be done as a series of accesses with smaller sizes.
> > -         */
> > -        unsigned max_access_size;
> > -        /* If true, unaligned accesses are supported.  Otherwise all accesses
> > -         * are converted to (possibly multiple) naturally aligned accesses.
> > -         */
> > -         bool unaligned;
> > -    } impl;
> > +    MemoryRegionInternalConstraints impl;
> >
> >       /* If .read and .write are not present, old_portio may be used for
> >        * backwards compatibility with old portio registration
> > @@ -105,10 +114,10 @@ struct MemoryRegionOps {
> >       const MemoryRegionMmio old_mmio;
> >   };
> >
> > -typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> > -typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> > +typedef struct _CoalescedMemoryRange CoalescedMemoryRange;
> > +typedef struct _MemoryRegionIoeventfd MemoryRegionIoeventfd;
> >
> > -struct MemoryRegion {
> > +struct _MemoryRegion {
> >       /* All fields are private - violators will be prosecuted */
> >       const MemoryRegionOps *ops;
> >       void *opaque;
> > @@ -127,16 +136,16 @@ struct MemoryRegion {
> >       target_phys_addr_t alias_offset;
> >       unsigned priority;
> >       bool may_overlap;
> > -    QTAILQ_HEAD(subregions, MemoryRegion) subregions;
> > -    QTAILQ_ENTRY(MemoryRegion) subregions_link;
> > -    QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
> > +    QTAILQ_HEAD(subregions, _MemoryRegion) subregions;
> > +    QTAILQ_ENTRY(_MemoryRegion) subregions_link;
> > +    QTAILQ_HEAD(coalesced_ranges, _CoalescedMemoryRange) coalesced;
> >       const char *name;
> >       uint8_t dirty_log_mask;
> >       unsigned ioeventfd_nb;
> >       MemoryRegionIoeventfd *ioeventfds;
> >   };
> >
> > -struct MemoryRegionPortio {
> > +struct _MemoryRegionPortio {
> >       uint32_t offset;
> >       uint32_t len;
> >       unsigned size;