[llvm-commits] [llvm] r44705 - in /llvm/trunk: include/llvm/Support/StringPool.h lib/Support/StringPool.cpp

[Reid Spencer]

Hi Gordon,

Nice addition.

Some minor feedback for you below ...

On Sat, 2007-12-08 at 17:07 +0000, Gordon Henriksen wrote:
> Author: gordon
> Date: Sat Dec  8 11:07:47 2007
> New Revision: 44705
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=44705&view=rev
> Log:
> Adding a StringPool data structure, which GC will use.
> 
> Added:
>     llvm/trunk/include/llvm/Support/StringPool.h
>     llvm/trunk/lib/Support/StringPool.cpp
> 
> Added: llvm/trunk/include/llvm/Support/StringPool.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/StringPool.h?rev=44705&view=auto
> 
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/StringPool.h (added)
> +++ llvm/trunk/include/llvm/Support/StringPool.h Sat Dec  8 11:07:47 2007
> @@ -0,0 +1,135 @@
> +//===-- StringPool.h - Interned string pool -------------------------------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file was developed by Gordon Henriksen and is distributed under the
> +// University of Illinois Open Source License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file declares an interned string pool, which helps reduce the cost of
> +// strings by using the same storage for identical strings.
> +// 
> +// To intern a string:
> +// 
> +//   StringPool Pool;
> +//   PooledStringPtr Str = Pool.intern("wakka wakka");
> +// 
> +// To use the value of an interned string, use operator bool and operator*:
> +// 
> +//   if (Str)
> +//     cerr << "the string is" << *Str << "\n";
> +// 
> +// Pooled strings are immutable, but you can change a PooledStringPtr to point
> +// to another instance. So that interned strings can eventually be freed,
> +// strings in the string pool are reference-counted (automatically).
> +// 
> +//===----------------------------------------------------------------------===//
> +
> +#ifndef LLVM_SUPPORT_STRINGPOOL_H
> +#define LLVM_SUPPORT_STRINGPOOL_H
> +
> +#include <llvm/ADT/StringMap.h>
> +#include <new>
> +#include <cassert>
> +
> +namespace llvm {
> +
> +  class PooledStringPtr;
> +
> +  /// StringPool - An interned string pool. Use the intern method to add a
> +  /// string. Strings are removed automatically as PooledStringPtrs are
> +  /// destroyed.
> +  class StringPool {
> +    struct PooledString {
> +      StringPool *Pool;  ///< So the string can remove itself.
> +      unsigned Refcount; ///< Number of referencing PooledStringPtrs.
> +      
> +    public:
> +      PooledString() : Pool(0), Refcount(0) { }
> +    };

Since you have added doxygen comments for the structure's data members,
why not document the structure itself as well? This will lead to less
confusing documentation.

> +    
> +    friend class PooledStringPtr;
> +    
> +    typedef StringMap<PooledString> table_t;
> +    typedef StringMapEntry<PooledString> entry_t;
> +    table_t InternTable;
> +    
> +  public:
> +    StringPool();
> +    ~StringPool();
> +    
> +    PooledStringPtr intern(const char *Begin, const char *End);
> +    inline PooledStringPtr intern(const char *Str);

These two methods are primary interfaces to the StringPool, aren't they?
Shouldn't they be documented with doxygen comments?
> +  };
> +  
> +  /// PooledStringPtr - A pointer to an interned string. Use operator bool to
> +  /// test whether the pointer is valid, and operator * to get the string if so.
> +  /// This is a lightweight value class with storage requirements equivalent to
> +  /// a single pointer, but it does have reference-counting overhead when
> +  /// copied.
> +  class PooledStringPtr {
> +    typedef StringPool::entry_t entry_t;
> +    entry_t *S;
> +    
> +  public:
> +    PooledStringPtr() : S(0) {}
> +    
> +    explicit PooledStringPtr(entry_t *E) : S(E) {
> +      if (S) ++S->getValue().Refcount;
> +    }
> +    
> +    PooledStringPtr(const PooledStringPtr &That) : S(That.S) {
> +      if (S) ++S->getValue().Refcount;
> +    }
> +    
> +    PooledStringPtr &operator=(const PooledStringPtr &That) {
> +      if (S != That.S) {
> +        clear();
> +        S = That.S;
> +        if (S) ++S->getValue().Refcount;
> +      }
> +      return *this;
> +    }
> +    
> +    void clear() {
> +      if (!S)
> +        return;
> +      if (--S->getValue().Refcount == 0) {
> +        S->getValue().Pool->InternTable.remove(S);
> +        delete S;
> +      }
> +      S = 0;
> +    }
> +    
> +    ~PooledStringPtr() { clear(); }
> +    
> +    inline const char *begin() const {
> +      assert(*this && "Attempt to dereference empty PooledStringPtr!");
> +      return S->getKeyData();
> +    }
> +    
> +    inline const char *end() const {
> +      assert(*this && "Attempt to dereference empty PooledStringPtr!");
> +      return S->getKeyData() + S->getKeyLength();
> +    }
> +    
> +    inline unsigned size() const {
> +      assert(*this && "Attempt to dereference empty PooledStringPtr!");
> +      return S->getKeyLength();
> +    }
> +    
> +    inline const char *operator*() const { return begin(); }
> +    inline operator bool() const { return S != 0; }
> +    
> +    inline bool operator==(const PooledStringPtr &That) { return S == That.S; }
> +    inline bool operator!=(const PooledStringPtr &That) { return S != That.S; }
> +  };
> +  
> +  PooledStringPtr StringPool::intern(const char *Str) {
> +    return intern(Str, Str + strlen(Str));

Maybe use strnlen(3) here to guard against Str not being null
terminated ?

> +  }
> +
> +} // End llvm namespace
> +
> +#endif
> 
> Added: llvm/trunk/lib/Support/StringPool.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/StringPool.cpp?rev=44705&view=auto
> 
> ==============================================================================
> --- llvm/trunk/lib/Support/StringPool.cpp (added)
> +++ llvm/trunk/lib/Support/StringPool.cpp Sat Dec  8 11:07:47 2007
> @@ -0,0 +1,35 @@
> +//===-- StringPool.cpp - Interned string pool -----------------------------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file was developed by Gordon Henriksen and is distributed under the
> +// University of Illinois Open Source License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file implements the StringPool class.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#include "llvm/Support/StringPool.h"
> +#include "llvm/Support/Streams.h"
> +
> +using namespace llvm;
> +
> +StringPool::StringPool() {}
> +
> +StringPool::~StringPool() {
> +  assert(InternTable.empty() && "PooledStringPtr leaked!");
> +}
> +
> +PooledStringPtr StringPool::intern(const char *Begin, const char *End) {
> +  table_t::iterator I = InternTable.find(Begin, End);
> +  if (I != InternTable.end())
> +    return PooledStringPtr(&*I);
> +  
> +  entry_t *S = entry_t::Create(Begin, End);
> +  S->getValue().Pool = this;
> +  InternTable.insert(S);
> +  
> +  return PooledStringPtr(S);
> +}
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits