labath accepted this revision.
labath added inline comments.
================
Comment at: source/Utility/ConstString.cpp:49
+ // pointer, we don't need the lock.
const StringPoolEntryType &entry = GetStringMapEntryFromKeyData(ccstr);
return entry.getKey().size();
----------------
labath wrote:
> scott.smith wrote:
> > zturner wrote:
> > > Why do we even have this function which digs into the `StringMap`
> > > internals rather than just calling existing `StringMap` member functions?
> > > Can Can we just delete `GetStringMapEntryFromKeyData` entirely and use
> > > `StringMap::find`?
> > Probably performance. If we have to call Find, then we have to call hash,
> > fault in the appropriate bucket, and then finally return the entry that we
> > already have in hand. Plus we'd need the lock.
> >
> > Can we just delete GetStringMapEntryFromKeyData entirely and use
> > StringMap::find?
> Unfortunately, I don't think that's possible. `StringMap::find` expects a
> StringRef. In order to construct that, we need to know the length of the
> string, and we're back where we started :(
>
> In reality, this is doing a very different operation than find (which takes a
> random string and checks whether it's in the map) -- this takes a string
> which we know to be in the map and get its size.
>
> It will take some rearchitecting of the ConstString class to get rid of this
> hack. Probably it could be fixed by ConstString storing a StringMap::iterator
> instead of the raw pointer. In any case, that seems out of scope of this
> change.
Cool, I didn't notice that one when looking for it. I guess at this point we
can just delete our copy of `GetStringMapEntryFromKeyData` completely and call
the StringPool's version instead.
Repository:
rL LLVM
https://reviews.llvm.org/D32306
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
