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();
----------------
scott.smith wrote:
> labath wrote:
> > 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.
> It will push a lot of lines past the 80 char limit. Do you want me to make
> that change? If not, can you submit this one since I do not have commit
> access? Thanks!
Ok, nevermind, let's leave that as is for now. thanks for the patch.
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
