[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

[Dan Gohman]

On Jul 9, 2008, at 11:11 PM, Chris Lattner wrote:
>> --- 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.

Ok.

>  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.

ilist_iterator is also used.

>> +++ 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.

It stood for "allocation" when I first named it, but it evolved
a bit since then.

>
>
>> +#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.

Because alist_node.h includes it? Ok.

>
>
>> +// 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.

The whole situation in LLVM with iterators and pointers being
two-way implicit-convertable has its conveniences, but it has
its dark sides too. If the above aren't declared, overload
resolution will make different choices.

>
>
>> +/// 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?

Currently, alist is mostly just like ilist except that it doesn't  
require
the element types to provide their own next+prev pointers. And it's
Recycler-friendly.

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

Ok. I'll think about better names, as the design continues to evolve.

>
>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> = 
>> =====================================================================
>> --- 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.

Ok.

>
>
>> +/// 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.

I'll think about it. Right now it's convenient for Recycler to
"know" about alist_nodes, so that it can hide the details from
its users.

>
>
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> = 
>> =====================================================================
>> --- 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.

Ok.

Dan