[llvm-commits] [llvm] r53210 - in /llvm/trunk: include/llvm/ADT/alist.h include/llvm/ADT/alist_node.h include/llvm/ADT/ilist.h include/llvm/Support/Recycler.h include/llvm/Support/RecyclingAllocator.h include/llvm/SymbolTableListTraits.h lib/Support/Allocator.cpp

[Chris Lattner]

On Jul 7, 2008, at 3:58 PM, Dan Gohman wrote:

> Author: djg
> Date: Mon Jul  7 17:58:06 2008
> New Revision: 53210
>
> URL: http://llvm.org/viewvc/llvm-project?rev=53210&view=rev
> Log:
> Add some basic Pool-allocation infrastructure. This adds a Recycler  
> class,
> for handling bookkeeping for deleted objects, as well as the alist  
> class
> template, for keeping lists of objects allocated from Recyclers, and  
> some
> related utilities.

This is very nice Dan.  Some minor comments:

> Added: llvm/trunk/include/llvm/ADT/alist_node.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/alist_node.h?rev=53210&view=auto
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/include/llvm/ADT/alist_node.h (added)
> +++ llvm/trunk/include/llvm/ADT/alist_node.h Mon Jul  7 17:58:06 2008
> @@ -0,0 +1,126 @@
> +//==- llvm/ADT/alist_node.h - Next/Prev helper class for alist --- 
> *- C++ -*-==//
> +//

>
> +
> +#include <cassert>
> +#include "llvm/ADT/ilist.h"
> +#include "llvm/Support/AlignOf.h"
> +#include "llvm/Support/DataTypes.h"

Please move the <cassert> #include to the end.  I think you only have  
the ilist #include for the partial specialization of ilist_traits.   
You can just forward declare ilist_traits and remove the #include if  
this is the case.

I really like your use of partial specialization here, very nice and  
tidy.

> +++ llvm/trunk/include/llvm/ADT/alist.h Mon Jul  7 17:58:06 2008
> @@ -0,0 +1,292 @@
> +//==- llvm/ADT/alist.h - Linked lists with hooks ----------------- 
> *- C++ -*-==//

What is the "a" stand for in alist?  please mention it here or in the  
file comment.

> +#ifndef LLVM_ADT_ALIST_H
> +#define LLVM_ADT_ALIST_H
> +
> +#include <cassert>
> +#include "llvm/ADT/alist_node.h"
> +#include "llvm/ADT/STLExtras.h"

cassert isn't needed.

> +// do not implement. this is to catch errors when people try to use
> +// them as random access iterators

isn't it better to just not implement them?  Then people will get a  
compile time error instead of a link-time one.

> +/// alist - This class is an ilist-style container that automatically
> +/// adds the next/prev pointers. It is designed to work in  
> cooperation
> +/// with <llvm/Support/Recycler.h>.

Would it be possible to replace ilist with this or merge alist and  
ilist?  When would someone use ilist vs alist?  When would someone use  
alist vs ilist?

FWIW, the name 'alist' is somewhat hard for me, I keep typing 'alias'.  
Maybe it's a form of finger/brain autocomplete :)

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/include/llvm/Support/Recycler.h (added)
> +++ llvm/trunk/include/llvm/Support/Recycler.h Mon Jul  7 17:58:06  
> 2008

>
> +#ifndef LLVM_SUPPORT_RECYCLER_H
> +#define LLVM_SUPPORT_RECYCLER_H
> +
> +#include <cassert>
> +#include "llvm/ADT/alist_node.h"

<cassert> is dead.  System includes should be at end of #include list.

> +/// Recycler - This class manages a linked-list of deallocated nodes
> +/// and facilitates reusing deallocated memory in place of allocating
> +/// new memory. The objects it allocates are stored in alist_node
> +/// containers, so they may be used in alists.
> +///

Ok, if this is the motivation for alist, I have to ask: isn't it  
easier to just not try to use pointers in the nodes?  Why not blast  
over the first two words of the recycled memory with next/prev  
pointers regardless of what the type is?  It seems that doing so would  
simplify the design fairly substantially.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/include/llvm/Support/RecyclingAllocator.h (added)
> +++ llvm/trunk/include/llvm/Support/RecyclingAllocator.h Mon Jul  7  
> 17:58:06 2008
> +
> +#include <cassert>
> +#include "llvm/Support/Recycler.h"
> +#include "llvm/ADT/STLExtras.h"

cassert and STLExtras.h are both redundant.

>
> +
> +namespace llvm {
> +
> +/// RecyclingAllocator - This class wraps an Allocator, adding the
> +/// functionality of recycling deleted objects.
> +///
> +template<class AllocatorType, class T, class LargestT = T>
> +class RecyclingAllocator {

Very nice!

-Chris