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@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
