Re: [RFC, PATCH] Split pool_allocator and create a new object_allocator

[Richard Sandiford <rdsandiford@googlemail.com>]

Martin Liška <mliska@suse.cz> writes:
> @@ -136,18 +135,18 @@ private:
>  	int64_t align_i;
>        } u;
>  
> -    static inline allocation_object<U> *
> +    static inline allocation_object*

space before "*"

>      get_instance (void *data_ptr)
>      {
> -      return (allocation_object<U> *)(((char *)(data_ptr))
> -				      - offsetof (allocation_object<U>,
> +      return (allocation_object *)(((char *)(data_ptr))
> +				      - offsetof (allocation_object,
>  						  u.data));

space between ")" and "(".

>      }
>  
> -    static inline U *
> +    static inline void*
>      get_data (void *instance_ptr)
>      {
> -      return (U*)(((allocation_object<U> *) instance_ptr)->u.data);
> +      return (void*)(((allocation_object *) instance_ptr)->u.data);

same 2 comments here, although maybe dropping the cast would be better?

> @@ -387,11 +349,11 @@ pool_allocator<T>::allocate ()
>        /* We now know that we can take the first elt off the virgin list and
>  	 put it on the returned list.  */
>        block = m_virgin_free_list;
> -      header = (allocation_pool_list*) allocation_object<T>::get_data (block);
> +      header = (allocation_pool_list*) allocation_object::get_data (block);

Space before "*".  I'll not list out the others :-)

> @@ -408,36 +370,34 @@ pool_allocator<T>::allocate ()
>  
>  #ifdef ENABLE_CHECKING
>    /* Set the ID for element.  */
> -  allocation_object<T>::get_instance (header)->id = m_id;
> +  allocation_object::get_instance (header)->id = m_id;
>  #endif
>    VALGRIND_DISCARD (VALGRIND_MAKE_MEM_UNDEFINED (header, size));
>  
> -  /* Call default constructor.  */
> -  return (T *)(header);
> +  return (void *)(header);

Same comment about cast to void *.

> diff --git a/gcc/asan.c b/gcc/asan.c
> index e89817e..dabd6f1 100644
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -362,20 +362,20 @@ struct asan_mem_ref
>    /* Pool allocation new operator.  */
>    inline void *operator new (size_t)
>    {
> -    return pool.allocate ();
> +    return ::new (pool.allocate ()) asan_mem_ref ();
>    }
>  
>    /* Delete operator utilizing pool allocation.  */
>    inline void operator delete (void *ptr)
>    {
> -    pool.remove ((asan_mem_ref *) ptr);
> +    pool.remove (ptr);
>    }
>  
>    /* Memory allocation pool.  */
> -  static pool_allocator<asan_mem_ref> pool;
> +  static pool_allocator pool;
>  };

I'm probably going over old ground/wounds, sorry, but what's the benefit
of having this sort of pattern?  Why not simply have object_allocators
and make callers use pool.allocate () and pool.remove (x) (with pool.remove
calling the destructor) instead of new and delete?  It feels wrong to me
to tie the data type to a particular allocation object like this.
And using the pool allocator functions directly has the nice property
that you can tell when a delete/remove isn't necessary because the pool
itself is being cleared.

Thanks,
Richard