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

[Gordon Henriksen]

Hi Reid. Nice to hear from you.

On 2007-12-08, at 14:43, Reid Spencer wrote:

> Hi Gordon,
>
> Nice addition.

Thanks.

> Some minor feedback for you below ...
>
> On Sat, 2007-12-08 at 17:07 +0000, Gordon Henriksen wrote:
>
>> +    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.

Okay.

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

Okay.

>> +  PooledStringPtr StringPool::intern(const char *Str) {
>> +    return intern(Str, Str + strlen(Str));
>
> Maybe use strnlen(3)

That function is sufficiently nonstandard to not exist on Darwin.

> here to guard against Str not being null terminated ?

This is a convenience specifically for null-terminated strings, so I'm  
not sure how defensive programming here is useful.

— Gordon