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

Wed Jun 8 10:04:16 PDT 2022

dblaikie added a comment.

>>> If the theory is that this should keep working even with the library changing without LLDB rebuild, then as I wrote above that theory needs double-checking first. And additionally a good question to ask would be if it's really a good idea to do incompatible implementation changes to a class that has half of its functionality inline.
>>
>> Sorry, I wasn't meaning to discuss dynamic library versioning issues/mismatches, just static/fully matched versions.
>
> 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.

>> 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.

^

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

https://reviews.llvm.org/D122974