[pulseaudio-discuss] Allowing anonymous structs and unions

[Arun Raghavan]

Hi Ahmed,

On 23 September 2015 at 14:04, Ahmed S. Darwish <darwish.07 at gmail.com> wrote:
> Hi everyone,
>
> On Wed, Sep 23, 2015 at 02:27:55AM -0400, Alexander E. Patrakov wrote:
>> On 09/23/2015 02:12 AM, Tanu Kaskinen wrote:
>> >Hi all,
>> >
>> >We haven't been using anonymous structs and unions so far, because
>> >they've been non-standard in the past. C11 added support for them,
>> >however. GCC has supported them longer than that, I don't know about
>> >other compilers. Anonymous structs and unions are nice, and I think we
>> >should start allowing them. Any objections?
>>
>> I think it would be a good idea to send one small example patch that makes a
>> use of them, just to demonstrate where exactly in PulseAudio they would make
>> the code more elegant.
>>
>
> Sure, this pattern is used heavily in the memfd patches :-)
>
> ### Anonymous unions
>
> Used to provide the mempool with polymorphic memory region access
> regardless of the type of memory used:
>
>     union {
>         pa_mem mem;
>         union {
>             pa_shm shm;
>             pa_memfd memfd;
>             pa_privatemem privatemem;
>         } per_type;
>     };
>
> This way, a lot of the mempools code can transparently access its
> backend memory by using `mempool->mem.ptr' and `mempool->mem.size`.
>
> This also saves us from polluting the mempool memblock.c code with
> a huge number of switch cases ;-)

The more common way to do this is by embedding the parent struct. This
makes for cleaner code, because (a) we're now depending on the
compiler packing all the members of the union exactly the same (only
the parent is packed, and not the children), and (b) the packed
attribute that can generate less efficient code, and (c) attribute
packing is not a language feature, so how it works can vary across
compilers.

typedef struct {
  void *ptr;
  size_t size;
} pa_mem;

#define PA_MEM(a) ((pa_mem *) (a))
#define PA_MEM_PTR(a) (PA_MEM(a)->ptr)

typedef struct {
  pa_mem mem;
  int fd;
} pa_memfd;

typedef struct {
  pa_mem mem;
  int id;
  bool do_unlink;
} pa_shm;

Then you can basically do things like:

  pa_memfd *memfd;
  ...
  memcpy(PA_MEM_PTR(memfd), src, n);

You can see example usage of this pattern in pa_object, pa_msgobject,
etc. (and that includes some optional GLib-esque run-time type
checking too).

I'm not sure we want to add the extra weight of type checking to our
memory objects (especially if we're instantiating/destroying/castinf a
lot of them), but that argument is somewhat orthogonal.

> ### Anonymous structs
>
> They are also used in the memfd patches, but they're really used
> to minimize code repetitions and make relationships explicit.
>
> Basically, we want to build an inheritance relationship: pa_shm,
> pa_memfd, and pa_privatemem has the common parent pa_mem. We also
> want to make this relationship explicit since it's at the core of
> the anonymous polymorphic union mentioned above.
>
> To do so without introducing extra (compile-time) dereferences,
> the following patten is used:
>
> /* Parent anonymous structure representing a plain memory
>  * area and its size. Different structures inherit from this,
>  * representing different memory area types (e.g. Posix SHM,
>  * Linux memfds, or private mallocs).
>  *
>  * To inherit from this object, just include PA_MEM_STRUCT as
>  * the very first element of your structure definition. */
> #define PA_MEM_STRUCT struct {   \
>     void *ptr;                   \
>     size_t size;                 \
> } PA_GCC_PACKED
>
> typedef struct pa_mem {
>     PA_MEM_STRUCT;
> } pa_mem;
>
> This way, children pa_shm, pa_memfd, and pa_privatemem can be
> defined as follows:
>
> typedef struct pa_shm {
>     PA_MEM_STRUCT;
>     unsigned id;
>     bool do_unlink;
> } pa_shm;
>
> typedef struct pa_memfd {
>     PA_MEM_STRUCT;
>     int fd;
> } pa_memfd;
>
> typedef struct pa_privatemem {
>     PA_MEM_STRUCT;
> } pa_privatemem;
>

I'm not sure what this buys us over just embedding a pa_mem struct as
the first member of each of these structures (other than
pa_privatemem). The concerns about the packed attribute from earlier
in the mail also applies here.

Cheers,
Arun