[PATCH] D19025: Hash symbol names only once per global SymbolBody

[Rafael Espíndola via llvm-commits]

On 12 April 2016 at 14:45, Rui Ueyama <ruiu at google.com> wrote:
> ruiu added a comment.
>
> This is a very good change -- simple and effective.
>
>
> ================
> Comment at: ELF/SymbolTable.h:25
> @@ +24,3 @@
> +  SymName(StringRef Name) : Name(Name) {
> +    HashValue = llvm::DenseMapInfo<StringRef>::getHashValue(Name);
> +  }
> ----------------
> Do you have to use DenseMapInfo<StringRef>::getHashValue? I'd just use llvm::hash_value.

The nice thing about forwarding is that it makes sure we don't have to
worry about different types.

> ================
> Comment at: ELF/SymbolTable.h:30
> @@ +29,3 @@
> +  StringRef Name;
> +  unsigned HashValue;
> +};
> ----------------
> The size of unsigned int depends on the hosting platform, not the target platform, so SymName would have different hashes on 32-bit and 64-bit. Does this code produces the identical binaries when cross-linking on different machines?

unsigned is 32 bits everywhere we run. It is also the return of getHashValue.


> I'd name Hash instead of HashValue.

I will change.

OK to commit to lld first and then try to move to LLVM? In LLVM I
would probably code this as a template that stores T and and the hash
of T.

Cheers,
Rafael