[Lldb-commits] [PATCH] D50536: Fix: ConstString::GetConstCStringAndSetMangledCounterPart() should update the value if the key exists already

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz marked an inline comment as done.
sgraenitz added inline comments.


================
Comment at: source/Utility/ConstString.cpp:123-126
+      assert((map.find(demangled) == map.end() || strlen(map[demangled]) == 0 ||
+              map[demangled] == mangled_ccstr) &&
+             "The demangled string must have a unique counterpart or otherwise "
+             "it must be empty");
----------------
friss wrote:
> In an asserts build, this is going to d a bunch of additional lookups. Can we move the assert after the try_emplace and basically assert that entry.second == nullptr || !strcmp(mangled_ccstr, entry.second).
> 
> (It's unclear to me whether a pointer comparison is enough to test string equality in this case, do we know that everything passed to this function has been uniqued?)
> Can we move the assert after the try_emplace
Yes, right that's better.

> and basically assert that entry.second == nullptr || !strcmp(mangled_ccstr, entry.second)
I think it's worth allowing "overwrite empty string". There is code that does `my_const_string.SetCString("")` to indicate failure, maybe other code does it to indicate e.g. later..

> It's unclear to me whether a pointer comparison is enough
I think it's safe to keep it. The `ccstr` postfix is used all over to indicate that the code assumes a pool string. `Pool::GetConstCStringAndSetMangledCounterPart` is only used once in `ConstString` and the `Pool` class is defined locally in the cpp.


https://reviews.llvm.org/D50536