[Lldb-commits] [PATCH] D50536: Fix: ConstString::GetConstCStringAndSetMangledCounterPart() should update the value if the key exists already
Stefan Gränitz via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Aug 10 10:53:30 PDT 2018
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");
> 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.
More information about the lldb-commits