On Fri, 2020-09-18 at 07:49 +0200, Aldy Hernandez wrote:
> 
> On 9/18/20 3:43 AM, David Malcolm wrote:
> > On Thu, 2020-09-17 at 12:36 +0200, Aldy Hernandez via Gcc-patches
> > wrote:
> > > This is the irange storage class.  It is used to allocate the
> > > minimum
> > > amount of storage needed for a given irange.  Storage is
> > > automatically
> > > freed at destruction.
> > > 
> > > It is meant for long term storage, as opposed to int_range_max
> > > which
> > > is
> > > meant for intermediate temporary results on the stack.
> > > 
> > > The general gist is:
> > > 
> > >   irange_pool pool;
> > > 
> > >   // Allocate an irange of 5 sub-ranges.
> > >   irange *p = pool.allocate (5);
> > > 
> > >   // Allocate an irange of 3 sub-ranges.
> > >   irange *q = pool.allocate (3);
> > > 
> > >   // Allocate an irange with as many sub-ranges as are currently
> > >   // used in "some_other_range".
> > >   irange *r = pool.allocate (some_other_range);
> > 
> > FWIW my first thoughts reading this example were - "how do I
> > deallocate
> > these iranges?" and "do I need to call pool.deallocate on them, or
> > is
> > that done for me by the irange dtor?"
> 
> Thus the description front and center of the header file:
> 
> "Storage is automatically freed at destruction..."
> 
> > I think of a "pool allocator" as something that makes a small
> > number of
> > large allocation under the covers, and then uses that to serve
> > large
> > numbers of fixed sized small allocations and deallocations with
> > O(1)
> > using a free list.
> 
> Ah, I didn't know pool had a different meaning.

See e.g. gcc/alloc-pool.h


> > [...]
> > 
> > > +// This is the irange storage class.  It is used to allocate the
> > > +// minimum amount of storage needed for a given irange.  Storage
> > > is
> > > +// automatically freed at destruction.
> > 
> > "at destruction" of what object - the irange or the irange_pool?
> > Reading the code, it turns out to be "at destruction of the
> > irange_pool", and it turns out that irange_pool is an obstack under
> > the
> > covers (also called a "bump allocator") and thus, I believe, the
> > lifetime of the irange instances is that of the storage instance.
> 
> The sentence is talking about the storage class, so I thought it was 
> obvious that the destruction we talk about is the storage class
> itself. 
> I suppose if it isn't clear we could say:
> 
> "Storage is automatically freed at destruction of the storage class."


> > I think it would be clearer to name this "irange_obstack", or
> > somesuch.
> 
> I'd prefer something more generic.  We don't want to tie the name of
> the 
> allocator to the underlying implementation.  What if we later change
> to 
> malloc?  We'd have to change the name to irange_malloc.

> irange_allocator?  Or is there something more generically appropriate
> here?

How about "irange_bump_allocator?"   Rather long, but it expresses the
key point that the irange instances coming out of it don't have
independent lifetimes - their lifetimes are those of the allocator; the
client code doesn't need to find and clean up all of those individual
iranges, right?  (assuming I'm reading the code correctly)   (That name
also avoids mentioning the implementation detail that it uses obstack).

Sorry if I'm nitpicking; I think my high level thought here is that we
have various memory management strategies inside GCC (in no particular
order):
  - garbage collection
  - explicit malloc/free
  - explicit new/delete
  - explicit new/delete backed by allocation pools
  - RAII
  - bump allocators aka obstacks
and I like to be clear about what "owns" each object (responsibility
for cleanup, lifetimes, etc)

Hope this is constructive
Dave

Reply via email to