[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

>>> Finally, there have been already attempts to change the hash function to use the better non-zero seed (D97396 <https://reviews.llvm.org/D97396>), and they were reverted because something depended on the hash function not changing, so I don't see it that likely for the hash function to change just like that in a minor update.
>>
>> That something seems to have been another part of lldb - and that's the sort of change I'd like to enable/not make harder by avoiding more dependence on this ordering/hashing.
>>
>>> But if after all this that's still the theory, I guess StringMap could get an additional template parameter specifying the hash function, if it's actually worth the effort.
>>
>> Yeah, a little traits class or the like is roughly what I'd picture.
>
> ^

^ I think it's still worthwhile/necessary to separate LLDB's use case/hashing algorithm choice from LLVM's so LLVM's code can be changed to be more change resilient in a way that LLDB's cannot (eg: random seeds will never be usable by LLDB but may be for LLVM).

In D122974#3567444 <https://reviews.llvm.org/D122974#3567444>, @llunak wrote:

> In D122974#3567278 <https://reviews.llvm.org/D122974#3567278>, @dblaikie wrote:
>
>>> Then I still don't know what the problem is supposed to be. If the StringMap hash implementation ever changes, the necessary LLDB rebuild will detect this, the relevant LLDB parts will get adjusted and problem solved.
>>
>> What I mean is if the cache is used across statically linked versions - eg: cache is created, someone installs an update to lldb, then the cache is read back and misinterprets the hashes in the cache if the hash algorithm had changed between versions.
>
> That is not going to happen, that updated LLDB will be using a different cache version. That's what the part you're replying to means. You cannot have two LLDB builds using the same cache version but different hash implementations, as long as you do not ignore LLDB unittests failing.

Fair enough - probably good to have some commentary in the test cases that makes it really clear that if the test needs to be updated then the version needs to be updated. (is that patch already posted? Could you link to that comment from this review?)


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

https://reviews.llvm.org/D122974