[llvm-commits] [lld] r150547 - in /lld/trunk: include/lld/Core/SymbolTable.h lib/Core/SymbolTable.cpp

[Nick Kledzik]

On Feb 15, 2012, at 3:37 AM, Chris Lattner wrote:
> On Feb 14, 2012, at 4:50 PM, Nick Kledzik wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=150547&view=rev
>> Log:
>> use llvm::DenseMap instead of std::map
> 
> Great.  One funny thing sticks out at me though:
> 
>> +  struct StringRefMappingInfo {
>> +    static llvm::StringRef getEmptyKey() { return llvm::StringRef(); }
>> +    static llvm::StringRef getTombstoneKey() { return llvm::StringRef(" ", 0); }
>> +    static unsigned getHashValue(llvm::StringRef const val) {
>> +                                               return llvm::HashString(val); }
>> +    static bool isEqual(llvm::StringRef const lhs, 
>> +                        llvm::StringRef const rhs) { return lhs.equals(rhs); }
>> +  };
>> +	typedef llvm::DenseMap<llvm::StringRef, const Atom *, 
>> +                                           StringRefMappingInfo> NameToAtom;
> 
> Why a DenseMap of StringRef?  Are the strings that you are hashing nul terminated?  If so, you can save some space in the DenseMap by just keeping the char* as a key.
The atom model has Atom::name() returning a StringRef.  So, the resolver cannot rely on nul termination.


> Secondarily, if the ownership of your strings is not known (so the map needs to copy them) then it makes sense to use StringMap.  I assume that you have permanent pointers  around in mapped files, so you don't have to worry about this though.
Yes. The Atom owns the name StringRef permanently, so there is no need to copy the string content in to the symbol table. (Actually, Atoms are own by their File, and in practice the File maps in file content, so the strings are permanent).

-Nick