[pulseaudio-discuss] [PATCH 03/11] pulsecore: Transform pa_mempool_new() into a factory method

[Tanu Kaskinen]

On Sun, 2015-09-20 at 23:29 +0200, Ahmed S. Darwish wrote:
> diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
> index 9b6810d..aa64d01 100644
> --- a/src/pulsecore/memblock.c
> +++ b/src/pulsecore/memblock.c
> @@ -145,7 +145,14 @@ struct pa_mempool {
>      pa_semaphore *semaphore;
>      pa_mutex *mutex;
>  
> -    pa_shm memory;
> +    pa_mempool_type_t type;
> +    union {
> +        pa_shm shm;
> +    } per_type;
> +
> +    void *ptr;
> +    size_t size;

What are ptr and size? I'll probably figure that out when I continue
reading the changes, but a comment would be appropriate here.

> -pa_mempool* pa_mempool_new(bool shared, size_t size) {
> +static const char *pa_mempool_type_tostr(pa_mempool_type_t type) {

"_to_string" is the conventional suffix.

> +    switch (type) {
> +    case PA_MEMPOOL_SHARED_POSIX:
> +        return "shared posix-shm";
> +    case PA_MEMPOOL_SHARED_MEMFD:
> +        return "shared memfd";
> +    case PA_MEMPOOL_PRIVATE:
> +        return "private";
> +    default:
> +        pa_assert_not_reached();
> +    }

It's better to not have the default case, because that allows the
compiler to warn if the code is not updated when the enum is extended.
The assertion can be after the switch.

> +}
> +
> +pa_mempool *pa_mempool_new(pa_mempool_type_t type, size_t size) {
>      pa_mempool *p;
> +    size_t pa_mem_size;

The pa_ prefix should only be used for identifiers that are exported in
headers.

You could also avoid introducing any new variable by assigning to the
size variable the final size once it's known. Or is changing function
parameter values too ugly?

Apart from these nitpicks, looks good!

-- 
Tanu