[PATCH] D64640: PDB HashTable: Move TraitsT from class parameter to the methods that need it

Fri Jul 12 12:00:28 PDT 2019

thakis added a comment.

In D64640#1583265 <https://reviews.llvm.org/D64640#1583265>, @rnk wrote:

> > The traits object is only used by a few methods. Deserializing a hash
> >  table and walking it is possible without the traits object, so it
> >  shouldn't be required to build a dummy object for that use case.
>
> The traits are needed to do any actual hashing.

I believe that's not really true: The has key is always an uint32_t, and assuming that lookupKeyToStorageKey() and storageKeyToLookupKey() are inverses of each other (which currently is true for StringTableHashTraits; for NamedStreamMapTraits I'm not yet sure what the semantics for two streams with the same name are. I believe it's not allowed – passing two identical /natvis: params to lld seems to confuse everyone at least -- and then it'd be true there too). So I currently think (but could be wrong, I'm not sure yet) that the hashtable hash function is overly complicated and should always work just on the uint32_t key and there should be a convenience function to convert a string key to an uint32_t. But that's for another change.

> I think I have a bit of a preference for the code as it is before your change. Now all the hash lookup operations have to take an extra parameter, and the caller is responsible for managing the lifetime of a new object that probably should exactly match the lifetime of the hash table.

The callers did manage the traits object before as well. Maybe that wasn't necessary, but see PDBFileBuilder.h and llvm/include/llvm/DebugInfo/PDB/Native/NamedStreamMap.h on the lhs – the traits objects were already member variables. There are 3 distinct call sites to the lookup operations from non-test code.

> What do you think of having a traits-less HashTableView or HashTableBase and having HashTable inherit from it? Would that solve your use case?

https://reviews.llvm.org/D64428?id=208735 used to do this. It was imo much messier (see the "drat" comments for some unresolved issues with that approach, but even with them resolved it's pretty messy). I then left the hash table as is and added another traits class that was silently unused in https://reviews.llvm.org/D64428?id=209475 but didn't like that either (see "xxx blah" for an annoying issue there – a function that's silently unused can't be implemented nicely). I think this change is by far the nicest.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64640/new/

https://reviews.llvm.org/D64640